Allow disabling OpenSSL initialization#4
Conversation
henryr
left a comment
henryr
left a comment
There was a problem hiding this comment.
Looks fine to me. Couple of minor comments.
| } | ||
|
|
||
| if ((ctx->ssl_ctx = SSL_CTX_new(SSLv23_server_method())) == NULL) { | ||
| cry(fc(ctx), "SSL_CTX_new (server) error: %s", ssl_error()); |
There was a problem hiding this comment.
Can you add a suggestion that SSL_library_init() might not have been called?
There was a problem hiding this comment.
you mean in the case that SSL_CTX_new fails? Do you know if it actually fails if SSL_library_init has not been called?
There was a problem hiding this comment.
From what I've observed, SSL_CTX_new will fail if SSL_library_init has not been called. The docs I've read don't specifically mention otherwise.
There was a problem hiding this comment.
ok, seems like this returns a reasonable consistent error code, so I'll detect it and give a better message
squeasel.c
Outdated
| SSL_library_init(); | ||
| SSL_load_error_strings(); | ||
| // Initialize SSL library, unless the user has disabled this. | ||
| if (sq_strcasecmp(ctx->config[SSL_GLOBAL_INIT], "yes") == 0) { |
There was a problem hiding this comment.
This doesn't really deal with the initialization race issue, right? Reason being that if we can't guarantee serialization of this section and a hosting application initializing SSL, we also can't guarantee that the hosting application has initialized SSL before we get to line 4399. i.e. the onus is on the hosting app to initialize SSL before initializing Squeasel.
There was a problem hiding this comment.
Yea, the assumption is that the hosting app has to make sure they initialize SSL before they use squeasel
|
This patch LGTM. |
This reverts commit 895fb22. Change-Id: Iaff9915288a83eb031b4bb21e212804f54586d61
This reverts commit 1907157. Change-Id: I5fce708bc174322b5601dc10395b1ac42e7edd41
This reverts commit b578ce9. This goes back to explicitly depending on OpenSSL rather than opening it using dlopen. To recap history here: - originally, squeasel used dlopen() to load OpenSSL so that it was an optional dependency - dd40db5 switched to just using it explicitly rather than dlopen()ing to simplify the code and allow us to use the OpenSSL headers/macros more robustly. - b578ce9 reverted back to the dlopen() approach because RHEL 6.6 made a breaking change to the OpenSSL ABI. Thus, without dynamic linking, a binary built on 6.4 wouldn't work on 6.6 and vice versa. We wanted to support both operating systems, so dlopen() was an appropriate workaround. Now we're going back to the explicit approach because both Kudu and Impala (the only known embedders of Squeasel) have inherited their own explicit dependencies on OpenSSL. Additionally, el6.4 is old enough that we are less concerned about supporting it at this point. Change-Id: Ibd53e228195ab82bbfe39a99d88b0d796f9ab958
This adds a new option 'ssl_global_init' which allows the user to disable the initialization of the SSL libraries. This is important in the case that the embedding application is also using OpenSSL and wants to provide its own locking/thread-ID callbacks. If the application and Squeasel both try to provide these callbacks, then we may have a race during initialization or crashes after Squeasel is unloaded. Change-Id: I5e53a21250350d28ff881adfc9b8dab4e3af58cf
Change-Id: I24a583cfdaaf4e39dd373dda1be394a1a34b639e
|
@henryr does the revision look good to you? |
|
lgtm |
3 participants
@henryr @smukil please take a look (haven't tested this yet)