[openssl-dev] Review of PR #451 (thread-safety / builtin locks)

[Nico Williams]

Hi.  Below are my high-level comments about this PR.

Of course, first, thanks for doing this work; I hope you see it through!

Comments:

 - If support for systems older than Vista is needed, then you'll need
   an implementation of InitOnceExecuteOnce().  I can point at suitable
   implementations if need be.

   E.g.,
   https://github.com/heimdal/heimdal/blob/master/lib/base/heimbase.c#L406

   (Should be easy to massage into exactly InitOnceExecuteOnce().)


 - The thread-local API is problematic.

   The implementation uses locks only because it allows the caller of
   the setter to also pass in a destructor.

   Instead the API should be much more like the POSIX
   pthread_key_create(), pthread_getspecific(), pthread_setspecific(),
   and pthread_key_destroy().

   On Windows you should use DllMain() to process DLL_THREAD_DETACH to
   call destructors for the thread's keys.

   Then you'll be able to say "look ma', no locks here".

   For me this is critical and must be fixed as described.


 - What is the status of the old lock callback getters/setters after
   this?  I don't see any changes to that code.

   Dead code should be removed, thought you should leave the old
   functions just for backwards compatibility.

   But then again, if the app provides these callbacks in a non-threaded
   configuration of OpenSSL, then they ought to be used!


 - I don't feel confident that using rw locks is a good idea for the
   first iteration.

   When I wrote the commits in the thread_safety branch of my clone
   (https://github.com/nicowilliams/openssl) I didn't feel at all
   confident that OpenSSL's use of read/write locks was correct.  The
   reason is that the underlying locks are generally exclusive locks
   (when callbacks are provided), so the use of read vs write locks may
   be incorrect.

   Use of rw vs exclusive locks should be configurable for a while, and
   thourough testing should be done to ensure that rw locks will work.

   Also, exclusive locks are easier to use on Windows (as we can see in
   this PR, that's what's used on Windows).

   You might want to copy my once-initializer for the pthread case from
   the thread_safety branch of my clone.  This will prove useful for
   other things even if you don't retain support for using lock
   callbacks provided by the app (which is why I needed a once-
   initializer: so I could set callbacks once and only once).

   (It was probably silly for me to try to use lock callbacks if
   provided, in retrospect I see little value for that, except in the
   non-threaded configurations of OpenSSL.)


 - I don't understand why use different function names for the locking
   functions.  Can you explain?


 - I don't think critical sections are the right thing to do on Windows.

   A mutex on Windows would be better (but one needs a once-initializer
   to set those up correctly since there's no static initializer for
   mutexes on Windows).


 - I wonder if OpenSSL in non-threaded configuration could detect and
   adjust to transitions to threaded mode.  This would be heroic on the
   part of OpenSSL, so I won't insist on it :)

   It would be nice, but it can wait too.

   For the dynamically-linked case you can detect threaded-ness by using
   weak symbols for pthreads, where weak symbols are supported anyways,
   and there's no need to use dlopen().  Although the switch from
   not-threaded to threaded will require care (a once initializer), and
   in particular locks that were "owned" in the not-threaded case should
   become real locks and acquired so that they stay owned if the caller
   is the main thread, otherwise if the caller is not the main thread
   and the main thread owns OpenSSL locks, then you must return an
   error.

   I'm not sure how one would do this in the statically linked case.


Cheers,

Nico
--