Re: [Opensc-devel] memory leaks with engine_pkcs11

[Anthony F. <ant...@gm...>]

Greetings --

I've been using this patch for a few weeks, and it seems to be holding up
just fine (and this is with a use case that loads a new key every minute --
previously, I was leaking 2MB/hr...).

I don't see myself working on this much further, so I'd like to see this
branch integrated into the mainline (assuming that the primary custodians
aren't too appalled by the techniques applied).  As such, I've submitted a
pull request on github:

https://github.com/OpenSC/engine_pkcs11/pull/3

If I can make it more palatable in small ways, let me know, and I'll try to
do so.  Big changes are probably right out; if the current form is
unacceptable on a fundamental level, I'll just have to move on and use my
patched version.

Either way, thanks to everyone for providing me with a great base of code
to work with, warts and all.  :)

Best regards,
Anthony Foiani



On Tue, May 7, 2013 at 5:49 PM, Anthony Foiani <ant...@gm...>wrote:

> Greetings, all --
>
> On Thu, May 2, 2013 at 10:25 AM, Anthony Foiani
> <ant...@gm...> wrote:
> >
> >
> > On Thu, May 2, 2013 at 10:15 AM, Anthony Foiani
> > <ant...@gm...> wrote:
> >
> > > Or does the private key structure rely on information held in
> > > the enumeration?
> >
> > So apparently it does.
> >
> > > If the latter, do we need to somehow provide a way for the key to
> > > release the entire enumeration when the key itself is freed?
>
> I have a solution, but it's very ugly.
>
> Please take a look at this branch:
>
>   https://github.com/tkil/engine_pkcs11/commits/ajf-fixes-201305
>
> It's mostly cleanup patches, but the punchline is in this commit:
>
>
> https://github.com/tkil/engine_pkcs11/commit/91133b719c71d0c92c233a0c29b0d5b94c4ee102
>   (or: http://preview.tinyurl.com/cdabyyf)
>
> The commit message tells the story:
>
> Without this patch:
>
>   ==25864== LEAK SUMMARY:
>   ==25864==    definitely lost: 4,920 bytes in 27 blocks
>   ==25864==    indirectly lost: 147,092 bytes in 3,137 blocks
>   ==25864==      possibly lost: 0 bytes in 0 blocks
>   ==25864==    still reachable: 14,231 bytes in 429 blocks
>   ==25864==         suppressed: 0 bytes in 0 blocks
>
> With this patch:
>
>   ==25989== LEAK SUMMARY:
>   ==25989==    definitely lost: 4,600 bytes in 23 blocks
>   ==25989==    indirectly lost: 328 bytes in 5 blocks
>   ==25989==      possibly lost: 0 bytes in 0 blocks
>   ==25989==    still reachable: 14,231 bytes in 429 blocks
>   ==25989==         suppressed: 0 bytes in 0 blocks
>
> Note the change in "indirectly lost".
>
> The ugly part is that I could not find any way to do it transparently.
>
> When we get a key through the OpenSSL engine interface, all we get
> back is an EVP_PKEY*.  I looked, and it doesn't seem that there's any
> way to add on arbitrary data; even if there were, there's no obvious
> way for external/third-party programs to modify the set of methods
> used by a given key.
>
> If there were, the right answer would have been:
>
> 1. Add the pointer to list of slots, and count of slots, as auxiliary
> data on the EVP_PKEY;
> 2. Modify the pkey_free method to free those and then chain to the
> original.
>
> I couldn't find anywhere to stick (1), and (2) was blocked because
> that method is behind an opaque pointer (the structure itself is
> defined in the openssl source package under crypto/asn1/asn1_locl.h,
> but that is not installed into the system headers directory, and it's
> explicitly mentioned that it's not for external use.)
>
> There are ways to get a pointer to the methods, duplicate it, and set
> a new free function -- but no way to get the original free function
> (!), so I couldn't chain to the original.
>
> Maybe I just missed something.
>
> It looks like someone else had a similar idea; in the hw_pcsk11.c
> file, there was an "#if 0" block with something that tries to chain
> the rsa_finish method ... but it was obviously not in use.
>
> Instead, I modified the engine to keep a separate list of EVP_PKEY* ->
> slot_list, slot_count mappings, and added an ENGINE_ctrl method to
> release the slot data given an EVP_PKEY*.
>
> I thought that I would still have to free the EVP_PKEY*, but it turns
> out that freeing the slots frees the keys:
>
> PKCS11_release_all_slots calls
> pkcs11_release_slot, which calls
> pkcs11_destroy_token, which calls
> pkcs11_destroy_keys, which calls
> EVP_PKEY_free
>
> This is also the reason that I could not just release the slots before
> returning the EVP_PKEY* out of engine_pkcs11.c:pkcs11_load_key
>
> Anyway.  Hopefully others will find this useful.  I can submit a
> proper pull request if you would like.
>
> Best regards,
> Anthony Foiani
>