Add an atexit handler to bypass calls into ERR_ string routines. #35925
Conversation
This works around Ubuntu's apparent lack of NO_ATEXIT support in their build of OpenSSL.
|
|
||
| // Now, spin until existing calls all finish, and we can move on with shutdown. | ||
| int x; | ||
| while ((x = __atomic_load_n(&g_str_read_count, __ATOMIC_SEQ_CST))) |
There was a problem hiding this comment.
should this do something like sleep(1) to avoid monopolizing a core?
|
|
||
| // Now, spin until existing calls all finish, and we can move on with shutdown. | ||
| int x; | ||
| while ((x = __atomic_load_n(&g_str_read_count, __ATOMIC_SEQ_CST))) |
There was a problem hiding this comment.
There is a race between this method and the CryptoNative_ErrReasonErrorString (the other one too). The CryptoNative_ErrReasonErrorString can check the g_err_unloaded before we set it here and the __atomic_load_n can load the g_str_read_count before the CryptoNative_ErrReasonErrorString updates it, thus leaving us with the same problem that we are trying to fix. And even if there was not a race, it would not work reliably e.g. on ARM with its weak memory model, c++ volatile is just an indication to the compiler that it should perform each read / write. It doesn't add any memory barriers. So the thread calling the error getting methods doesn't have to see the g_err_unloaded change in a limited time.
It seems that we should use regular lock (pthread_mutex) around setting the g_err_unloaded here and in both ERR_reason_error_string and CryptoNative_ErrErrorStringN around the check of g_err_unloaded and calls to openssl. Also, the g_err_unloaded can then be just volatile, the locks will provide the necessary barriers.
There was a problem hiding this comment.
Yep, good points. I started with a rwlock, but that's not in (g)libc, so I had to take a direct dependency on libpthread. Since we haven't anywhere else I then backed that out and came up with this, but with the usual "cleverness" consequences of missing something.
So I'll just turn it back into a mutex. These routines are only hit on loading exception messages, so it shouldn't be too much of a penalty.
There was a problem hiding this comment.
libcoreclr.so already depends on libpthread, so it is not a big deal if we add dependency here too.
| // Set this first, which stops new callers from using the error string tables. | ||
| g_err_unloaded = 1; | ||
|
|
||
| // Now, spin until existing calls all finish, and we can move on with shutdown. |
There was a problem hiding this comment.
Is it possible such calls could block or otherwise take an unbounded amount of time?
There was a problem hiding this comment.
Nah, they are just a TryGetValue and a snprintf.
|
OK, I think this is ready for review again. |
ghost
locked as resolved and limited conversation to collaborators
This works around Ubuntu's apparent lack of NO_ATEXIT support in their
build of OpenSSL.
Fixes #34231.