Better document thread safety considerations

[jiridanek]

I would like to see some clarification in the documentation on the issue of thread safety.

README.md claims that the sodium_init, crypto_sign_keypair, crypto_box_keypair and random number generating functions are not thread safe.

src/libsodium/include/sodium/crypto_box.h says that crypto_box_keypair is thread safe once sodium_init was called.

Issue #31 suggests that all except sodium_init might be thread safe once sodium_init has been called.

The original NaCl library is thread safe.

The desirable state IMO would be that all except sodium_init is thread safe, regardless of whether sodium_init has or has not been called.

[jedisct1]

Thread safety issues are due to randombytes(), and the original NaCl library has the same issue.

sodium_init() ensures that the descriptor has been initialized. Once done, everything is thread safe.

If calling a function once the app starts is really not an option, wrap crypto_sign_keypair, crypto_box_keypair and random number generating functions in locks.

[jiridanek]

Thanks for your explanation. I can use sodium_init(), no problem there. Still, I think that the information you provided me with should be placed in the README, so I am not taking the bug down.

Could you please point me to some reference that explains why randombytes() is not thread safe, for educational purposes?

[jedisct1]

fd is a global variable, not protected by any mutexes.

[jiridanek]

facepalm oh, I missed that.

One of the design requirements on the NaCl webpage states "Do not use global variables (i.e., static variables or variables defined outside functions) in C NaCl." I wonder what has happened to that worthy resolution.

[jedisct1]

Hopefully 0331a0d clarifies it.

[jiridanek]

Shouldn't the fd variable be declared as volatile?

[jedisct1]

volatile doesn't mean atomic, and it doesn't prevent the block from being executed by different threads at the same time. Even if assignments and tests of fd were atomic, it wouldn't prevent file descriptor leaks.

[jiridanek]

Shoul've been more specific. volatile means 'the value in memory can be changed by some external factor'. In this case, that would be some external thread. Without it, the compiler may put the value into a register and never actually look what is happening with it in the memory. I am pretty much refering to this StackOverflow discussion.

Volatile is also needed in threaded code when you are playing with data that isn't concurrency protected. And yes there are valid times to be doing that, you can for example write a thread safe circular message queue without needing explicit concurrency protection, but it will need volatiles.

In normal setting one uses a threading library that inserts memory fences where it is needed. Here we don't have that so volatile may be appropriate.

[tarcieri]

On Thu, Nov 21, 2013 at 1:48 PM, jirkadanek notifications@github.comwrote:

Shoul've been more specific. volatile means 'the value in memory can be
changed by some external factor'. In this case, that would be some external
thread.

So to be clear here: you're talking about another thread reopening
/dev/urandom?

Wouldn't that leak file descriptors? Why would you want to support that?

Tony Arcieri

[jedisct1]

It would. And if you are intentionally limiting the number of file descriptors, it will fail. So, call sodium_init() only once as documented, or if you really need to call it more than once, add locks.