Re: [Opensc-devel] Memory leaks with libp11 and engine-pkcs11

[Markus K. <ko...@rr...>]

On 09/26/2013 11:36 PM, Douglas E. Engert wrote:
> On 9/26/2013 3:02 PM, Markus Kötter wrote:
>> On 09/26/2013 09:21 PM, Douglas E. Engert wrote:
>>> On 9/26/2013 12:07 PM, Markus Koetter wrote:
...
>>> Is it possible to have the CTX contain an array of SLOTS,
>>> and the SLOTS and array of TOKENS? Thus at engine finish,
>>> any remaining SLOTS and TOKENS would be released.
>>
>> If you rely on engine finish to free memory, your application code is
>> broken.
>> You can't reload the engine, as re-loading gost will destroy openssl.
>> Reloading gost is default when using the pkcs11 engine.
>> Cleaning up on exit while leaking at runtime is cosmetics.
>>
>>
>> To get back to the question, it *may* be possible, if you
>>    * keep track of the CTX per PKCS11_KEY
>
> In libp11-int.h, for a key, the
> #define KEY2CTX(key)
> returns the CTX from the key.
>
>   KEY2CTX(key) returns the CTX

CTX is not a problem, there is a single CTX per engine.
As the CTX does not track the slots allocated, it does not helo.

>> but - moving the load_*_key procedure to p11 solves the overall problem.
>> In a c&p fashion, way less complex and intrusive.
>>
>>> What is also not clear is if the libp11 keeps track of sessions
>>> correctly.
>>> It looks like it only allows one session, but may not clean up
>>> correctly if a second session is opened.
>>
>> Session as in CTX?

If you think of session as in PKCS11 Session handle, I'm with you.

> No Sessions as in PKCS#11 sessions. The CTX has the session handle,
> but it can get replaced.

Currently - sessions are OpenSC global.
p11 closes the sessions on PKCS11_release_all_slots.

There is no reference counting for sessions.
Load 2 keys of the same card via the engine, free the slots for a key, 
and the handle for the others gets invalidated.

PKCS11_release_all_slots
pkcs11_release_slot
C_CloseAllSessions

EVP_SignFinal: No such file or directory
140627597526688:error:800280B3:PKCS11 
library:PKCS11_get_attribute:Session handle invalid:p11_attr.c:53:

So fixing the current memory leak without fixing the session handling 
results in dead sessions.

>>>> If you want tracking in p11, loading keys and certs (by slot/id, label)
>>>> has to happen in p11, not in the engine.

I moved the code to p11, works as before.
Had to restructure the pin handling a little, but left the use of 
UI_*_app_data as before.

But, as I said, free'ing memory kills sessions.

Not freeing memory keeps the sessions alive forever, they leak, 
accompanied by the memory.


I'll try to work something out.


MfG
Markus