`clone()` and `is_initialized()` about rust-cryptoki HOT 13 OPEN

I made a PR for a possible fix #152

from rust-cryptoki.

Hmm, there are now two PRs which attempt to fix the same issue - how come? Do you have a preference for which approach to take?

I'll need to go dig into our previous conversations and see why we chose to implement it like this, I know there are quite a few quirks of the PKCS11 spec and the way it's been actually implemented - stay tuned.

from rust-cryptoki.

I would prefer remove it completely but it's in the public api.

from rust-cryptoki.

I guess deprecating it in one version and then only removing it in a major version bump would be a conservative choice.

from rust-cryptoki.

Ok, I've had a bit of time to look at this now.

I'm not sure how to fix this. I'm not sure why initialized is in Pkcs11 and not in Pkcs11Impl or even stored at all.

initialized is in Pkcs11 because Pkcs11Impl is in an Arc, and therefore it can't be mutated. The sane solution for storing that bool in Pkcs11Impl would be to change the Arc into an RwLock or Arc<Mutex<Pkcs11Impl>>, but that's probably overkill. Also, the reason I was storing it was to avoid making any calls to the library - not necessarily because I had a request in that direction, it just seemed that judging based on the result of the call is somewhat risky and a would have a (much?) higher overhead. I say risky because if that call returns false for some reason other than "it really hasn't been initialized yet", then calling initialize again might break stuff - not sure, from the spec this seems to be implementation defined.

I'm also not sure why Pkcs11 should be cloned and not borrowed.

The reason might have to do with the general ugliness of having to deal with lifetimes if you need to keep a Pkcs11 somewhere in your app, and then pass along references through various parts of the app that will keep hold of the reference for arbitrary amounts of time. Easier to just clone and whenever the last entity is dropped you clean up.

The rationale for having that functionality is here: #77 - so I'd prefer keeping it in one form or another. @ximon18 - any thoughts? I'm somewhat torn between Arc<AtomicBool> and making a call to the backend.

from rust-cryptoki.

Arc<AtomicBool> will still leave race conditions when two threads try to initialize at the same time.

Only with a lock this can be prevented. See my PR #152 that uses a RWLock for a way to keep it in sync all the time.

from rust-cryptoki.

... a missing piece of functionality: the cryptoki crate has no equivalent of the pkcs11 crate `is_initialized() function.

That function works because that data structure does not allow clone().

from rust-cryptoki.

Sorry, only seeing this now. I don't have time to look at or consider this right now, I'll try and look at it on Monday.

from rust-cryptoki.

Arc<AtomicBool> will still leave race conditions when two threads try to initialize at the same time.

How so? Before you call the backend operation you can swap the flag with true - if you get back false you proceed with the operation, if not then you just return. Only one thread will then get to initialize. Am I missing something?

from rust-cryptoki.

If you set the flag before initializing other threads can get an unexpected 'not initialized' error from the library.

from rust-cryptoki.

Ahh, I see. Agree, it's not ideal, though not exactly a showstopper. Would you be ok with the RwLock implementation, then? I'll be reviewing that towards getting it in.

from rust-cryptoki.

Sure the RWLock implementation is fine. I just fixed the comments in the PR.

from rust-cryptoki.

Sorry, only seeing this now. I don't have time to look at or consider this right now, I'll try and look at it on Monday.

Ahem, not exactly Monday, or anytime near... but just noticed this issue in an old TODO list and as the linked PR #152 is merged and seems to contain the RWLock change, does this issue still need to be open?

from rust-cryptoki.