Imported public keys leak token session objects

[bukka]

Describe the bug

When a public key is stored, a new session object is created (C_CreateObject), but that object is never destroyed when the P11PROV_OBJ is freed. Under workloads that repeatedly import distinct public keys that get later stored (in my case TLS server side client certificate verification), this leaks one session object per unique key on the long-lived login session, which is never closed.

The freeing logic in p11prov_obj_free when only cache handle is released makes so private keys as the handle should not be destroyed but it doesn't make that much sense for imported objects as far as I see (correct me if I'm missing anything important).

To Reproduce

To reproduce, the code needs to store public key object repeatedly. In my case, it can be reproduced with client verification.

Expected behavior

The imported object handle should be destroyed - calling C_DestroyObject

Operating environment (please complete the following information):

• OS: Linux but shouldn't matter

Token and application used (please complete the following information):

• Device: SoftHSMv2 but shouldn't matter

[bukka]

I'm just testing a possible fix for this 264be78 in my fork which relies on two flags - dup ( 8e2cb12 ) and imported ( b4a1062 ) that I introduced to fix other issues earlier. If you think that something like this would make sense, I would be happy to create a proper PR.

[simo5]

The problem is knowing when it is ok to free this. I guess having to pay the price of re-import may be better than using a lot of memory ...

[simo5]

The patch is reasonable but should definitely use p11prov_try_session_ref() rather than p11prov_take_login_session()

[bukka]

So finally managed to put it together. It was much more stright forward in my old fork where I had dup flag but some things changed so I had to improvise a bit and needed to handle_owner concept to correctly handle the copy refs. I also used p11prov_try_session_ref as suggested. It's in #751. By the way that forked version in the mentioned commit was tested under big load with over 50 slots and resulted in huge decrease of memory (it's pretty much flat one and it was increasing significantly before) so it really helps.