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 -- _______________________________________________ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
