`clone()` and `is_initialized()`

[arjennienhuis]

Looking at he logic of initialize(), is_initialized() and finalize() I found out that Pkcs11::initialized can get out of sync with Pkcs11::impl_.

    #[test]
    fn test_clone_initialize() {
        let mut lib = Pkcs11::new(config::pkcs11_lib()).unwrap();
        let mut clone = lib.clone();
        lib.initialize(CInitializeArgs::OsThreads).unwrap();
        if (!clone.is_initialized()) {
            clone.initialize(CInitializeArgs::OsThreads).unwrap();
        }
    }

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.

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

[arjennienhuis]

I made a PR for a possible fix #152

[ionut-arm]

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.

[arjennienhuis]

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

[wiktor-k]

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

[ionut-arm]

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: https://github.com/parallaxsecond/rust-cryptoki/issues/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.

[arjennienhuis]

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.

[arjennienhuis]

... 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().

[ximon18]

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.

[ionut-arm]

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?

[arjennienhuis]

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

[ionut-arm]

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.

[arjennienhuis]

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

[ximon18]

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?

[hug-dev]

Closing then for now! Re-open if I was wrong :)