From 1d3364ac9d2bcd2d194f0afa0d301456d0d4e276 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 11 Jun 2014 20:52:15 +0200 Subject: [PATCH 1/5] netops: init OpenSSL once under lock The OpenSSL init functions are not reentrant, which means that running multiple fetches in parallel can cause us to crash. Use a mutex to init OpenSSL, and since we're adding this extra checks, init it only once. --- src/global.c | 3 +++ src/global.h | 2 ++ src/netops.c | 34 ++++++++++++++++++++++++++++++++-- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/global.c b/src/global.c index 7da31853e..fe410587e 100644 --- a/src/global.c +++ b/src/global.c @@ -16,6 +16,9 @@ git_mutex git__mwindow_mutex; #define MAX_SHUTDOWN_CB 8 +git_mutex git__ssl_mutex; +git_atomic git__ssl_init; + static git_global_shutdown_fn git__shutdown_callbacks[MAX_SHUTDOWN_CB]; static git_atomic git__n_shutdown_callbacks; static git_atomic git__n_inits; diff --git a/src/global.h b/src/global.h index 778250376..245f811e4 100644 --- a/src/global.h +++ b/src/global.h @@ -18,6 +18,8 @@ typedef struct { git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; +extern git_mutex git__ssl_mutex; +extern git_atomic git__ssl_init; #define GIT_GLOBAL (git__global_state()) diff --git a/src/netops.c b/src/netops.c index a0193a022..7bce159ad 100644 --- a/src/netops.c +++ b/src/netops.c @@ -33,6 +33,7 @@ #include "posix.h" #include "buffer.h" #include "http_parser.h" +#include "global.h" #ifdef GIT_WIN32 static void net_set_error(const char *str) @@ -386,12 +387,41 @@ cert_fail_name: return -1; } +/** + * The OpenSSL init functions are not reentrant so we need to init + * them under lock. + */ +static int init_ssl(void) +{ + if (git__ssl_init.val) + return 0; + + if (git_mutex_lock(&git__ssl_mutex) < 0) { + giterr_set(GITERR_OS, "failed to acquire ssl init lock"); + return -1; + } + + /* if we had to wait for the lock, someone else did it, we can return */ + if (git__ssl_init.val) + return 0; + + + SSL_library_init(); + SSL_load_error_strings(); + + git_atomic_set(&git__ssl_init, 1); + git_mutex_unlock(&git__ssl_mutex); + + return 0; +} + static int ssl_setup(gitno_socket *socket, const char *host, int flags) { int ret; - SSL_library_init(); - SSL_load_error_strings(); + if (init_ssl() < 0) + return -1; + socket->ssl.ctx = SSL_CTX_new(SSLv23_method()); if (socket->ssl.ctx == NULL) return ssl_set_error(&socket->ssl, 0); From 5fa0494328b78a1ce8dba983c3f8028d123a62a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 11 Jun 2014 23:19:48 +0200 Subject: [PATCH 2/5] ssl: use locking When using in a multithreaded context, OpenSSL needs to lock, and leaves it up to application to provide said locks. We were not doing this, and it's just luck that's kept us from crashing up to now. --- src/netops.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/netops.c b/src/netops.c index 7bce159ad..ba1a329e0 100644 --- a/src/netops.c +++ b/src/netops.c @@ -35,6 +35,11 @@ #include "http_parser.h" #include "global.h" +#if defined(GIT_SSL) && defined(GIT_THREADS) +/* OpenSSL wants us to keep an array of locks */ +static git_mutex *openssl_locks; +#endif + #ifdef GIT_WIN32 static void net_set_error(const char *str) { @@ -387,6 +392,24 @@ cert_fail_name: return -1; } +#ifdef GIT_THREADS +void openssl_locking_function(int mode, int n, const char *file, int line) +{ + int lock; + + GIT_UNUSED(file); + GIT_UNUSED(line); + + lock = mode & CRYPTO_LOCK; + + if (lock) { + git_mutex_lock(&openssl_locks[n]); + } else { + git_mutex_unlock(&openssl_locks[n]); + } +} +#endif + /** * The OpenSSL init functions are not reentrant so we need to init * them under lock. @@ -409,6 +432,25 @@ static int init_ssl(void) SSL_library_init(); SSL_load_error_strings(); +#ifdef GIT_THREADS + { + int num_locks, i; + + + CRYPTO_set_locking_callback(openssl_locking_function); + + num_locks = CRYPTO_num_locks(); + openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); + for (i = 0; i < num_locks; i++) { + if (git_mutex_init(&openssl_locks[i]) < 0) { + git_mutex_unlock(&git__ssl_mutex); + giterr_set(GITERR_SSL, "failed to init lock %d", i); + return -1; + } + } + } +#endif + git_atomic_set(&git__ssl_init, 1); git_mutex_unlock(&git__ssl_mutex); From cf15ac8aa96304a36699ae65398b7adac0d2ddde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 12 Jun 2014 03:20:34 +0200 Subject: [PATCH 3/5] ssl: cargo-cult thread safety OpenSSL's tests init everything in the main thread, so let's do that. --- src/global.c | 20 ++++++++++++++++++++ src/global.h | 5 +++++ src/netops.c | 43 +++++++++++++++++++++---------------------- src/netops.h | 1 - 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/src/global.c b/src/global.c index fe410587e..e9c940f2c 100644 --- a/src/global.c +++ b/src/global.c @@ -16,6 +16,11 @@ git_mutex git__mwindow_mutex; #define MAX_SHUTDOWN_CB 8 +#ifdef GIT_SSL +# include +SSL_CTX *git__ssl_ctx; +#endif + git_mutex git__ssl_mutex; git_atomic git__ssl_init; @@ -160,6 +165,15 @@ static pthread_key_t _tls_key; static pthread_once_t _once_init = PTHREAD_ONCE_INIT; int init_error = 0; +static void init_ssl(void) +{ +#ifdef GIT_SSL + SSL_load_error_strings(); + OpenSSL_add_ssl_algorithms(); + git__ssl_ctx = SSL_CTX_new(SSLv23_method()); +#endif +} + static void cb__free_status(void *st) { git__free(st); @@ -169,12 +183,18 @@ static void init_once(void) { if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0) return; + if ((init_error = git_mutex_init(&git__ssl_mutex)) != 0) + return; pthread_key_create(&_tls_key, &cb__free_status); + /* Initialize any other subsystems that have global state */ if ((init_error = git_hash_global_init()) >= 0) init_error = git_sysdir_global_init(); + /* OpenSSL needs to be initialized from the main thread */ + init_ssl(); + GIT_MEMORY_BARRIER; } diff --git a/src/global.h b/src/global.h index 245f811e4..8904e2de5 100644 --- a/src/global.h +++ b/src/global.h @@ -15,6 +15,11 @@ typedef struct { git_error error_t; } git_global_st; +#ifdef GIT_SSL +# include +extern SSL_CTX *git__ssl_ctx; +#endif + git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; diff --git a/src/netops.c b/src/netops.c index ba1a329e0..54804d418 100644 --- a/src/netops.c +++ b/src/netops.c @@ -163,7 +163,7 @@ void gitno_buffer_setup_callback( void gitno_buffer_setup(gitno_socket *socket, gitno_buffer *buf, char *data, size_t len) { #ifdef GIT_SSL - if (socket->ssl.ctx) { + if (socket->ssl.ssl) { gitno_buffer_setup_callback(socket, buf, data, len, gitno__recv_ssl, NULL); return; } @@ -208,7 +208,6 @@ static int gitno_ssl_teardown(gitno_ssl *ssl) ret = 0; SSL_free(ssl->ssl); - SSL_CTX_free(ssl->ctx); return ret; } @@ -428,30 +427,39 @@ static int init_ssl(void) if (git__ssl_init.val) return 0; - - SSL_library_init(); - SSL_load_error_strings(); + SSL_CTX_set_mode(git__ssl_ctx, SSL_MODE_AUTO_RETRY); + SSL_CTX_set_verify(git__ssl_ctx, SSL_VERIFY_NONE, NULL); + if (!SSL_CTX_set_default_verify_paths(git__ssl_ctx)) { + unsigned long err = ERR_get_error(); + giterr_set(GITERR_SSL, "failed to set verify paths: %s\n", ERR_error_string(err, NULL)); + return -1; + } #ifdef GIT_THREADS { int num_locks, i; - - CRYPTO_set_locking_callback(openssl_locking_function); - num_locks = CRYPTO_num_locks(); openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); + if (openssl_locks == NULL) { + git_mutex_unlock(&git__ssl_mutex); + return -1; + } + GITERR_CHECK_ALLOC(openssl_locks); + for (i = 0; i < num_locks; i++) { - if (git_mutex_init(&openssl_locks[i]) < 0) { + if (git_mutex_init(&openssl_locks[i]) != 0) { git_mutex_unlock(&git__ssl_mutex); giterr_set(GITERR_SSL, "failed to init lock %d", i); return -1; } } } + + CRYPTO_set_locking_callback(openssl_locking_function); #endif - git_atomic_set(&git__ssl_init, 1); + git_atomic_inc(&git__ssl_init); git_mutex_unlock(&git__ssl_mutex); return 0; @@ -464,16 +472,7 @@ static int ssl_setup(gitno_socket *socket, const char *host, int flags) if (init_ssl() < 0) return -1; - socket->ssl.ctx = SSL_CTX_new(SSLv23_method()); - if (socket->ssl.ctx == NULL) - return ssl_set_error(&socket->ssl, 0); - - SSL_CTX_set_mode(socket->ssl.ctx, SSL_MODE_AUTO_RETRY); - SSL_CTX_set_verify(socket->ssl.ctx, SSL_VERIFY_NONE, NULL); - if (!SSL_CTX_set_default_verify_paths(socket->ssl.ctx)) - return ssl_set_error(&socket->ssl, 0); - - socket->ssl.ssl = SSL_new(socket->ssl.ctx); + socket->ssl.ssl = SSL_new(git__ssl_ctx); if (socket->ssl.ssl == NULL) return ssl_set_error(&socket->ssl, 0); @@ -610,7 +609,7 @@ int gitno_send(gitno_socket *socket, const char *msg, size_t len, int flags) size_t off = 0; #ifdef GIT_SSL - if (socket->ssl.ctx) + if (socket->ssl.ssl) return gitno_send_ssl(&socket->ssl, msg, len, flags); #endif @@ -631,7 +630,7 @@ int gitno_send(gitno_socket *socket, const char *msg, size_t len, int flags) int gitno_close(gitno_socket *s) { #ifdef GIT_SSL - if (s->ssl.ctx && + if (s->ssl.ssl && gitno_ssl_teardown(&s->ssl) < 0) return -1; #endif diff --git a/src/netops.h b/src/netops.h index 8e3a2524f..dfb4ab7b4 100644 --- a/src/netops.h +++ b/src/netops.h @@ -16,7 +16,6 @@ struct gitno_ssl { #ifdef GIT_SSL - SSL_CTX *ctx; SSL *ssl; #else size_t dummy; From 8f897b6f2f4742a7d9189528e082cfe7cecd6f29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 12 Jun 2014 14:50:08 +0200 Subject: [PATCH 4/5] ssl: init also without threads --- src/global.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/global.c b/src/global.c index e9c940f2c..41428ec42 100644 --- a/src/global.c +++ b/src/global.c @@ -47,6 +47,15 @@ static void git__shutdown(void) } +static void init_ssl(void) +{ +#ifdef GIT_SSL + SSL_load_error_strings(); + OpenSSL_add_ssl_algorithms(); + git__ssl_ctx = SSL_CTX_new(SSLv23_method()); +#endif +} + /** * Handle the global state with TLS * @@ -165,15 +174,6 @@ static pthread_key_t _tls_key; static pthread_once_t _once_init = PTHREAD_ONCE_INIT; int init_error = 0; -static void init_ssl(void) -{ -#ifdef GIT_SSL - SSL_load_error_strings(); - OpenSSL_add_ssl_algorithms(); - git__ssl_ctx = SSL_CTX_new(SSLv23_method()); -#endif -} - static void cb__free_status(void *st) { git__free(st); @@ -248,6 +248,7 @@ static git_global_st __state; int git_threads_init(void) { + init_ssl(); git_atomic_inc(&git__n_inits); return 0; } From 081e76bac26e1ed6adafb402d15b30c622866d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 12 Jun 2014 16:20:52 +0200 Subject: [PATCH 5/5] ssl: init everything all the time Bring together all of the OpenSSL initialization to git_threads_init() so it's together and doesn't need locks. Moving it here also gives us libssh2 thread safety (when built against openssl). --- src/global.c | 53 +++++++++++++++++++++++++++++---- src/global.h | 2 -- src/netops.c | 83 ++-------------------------------------------------- 3 files changed, 51 insertions(+), 87 deletions(-) diff --git a/src/global.c b/src/global.c index 41428ec42..b144b050a 100644 --- a/src/global.c +++ b/src/global.c @@ -19,11 +19,9 @@ git_mutex git__mwindow_mutex; #ifdef GIT_SSL # include SSL_CTX *git__ssl_ctx; +static git_mutex *openssl_locks; #endif -git_mutex git__ssl_mutex; -git_atomic git__ssl_init; - static git_global_shutdown_fn git__shutdown_callbacks[MAX_SHUTDOWN_CB]; static git_atomic git__n_shutdown_callbacks; static git_atomic git__n_inits; @@ -47,12 +45,59 @@ static void git__shutdown(void) } +#if defined(GIT_THREADS) && defined(GIT_SSL) +void openssl_locking_function(int mode, int n, const char *file, int line) +{ + int lock; + + GIT_UNUSED(file); + GIT_UNUSED(line); + + lock = mode & CRYPTO_LOCK; + + if (lock) { + git_mutex_lock(&openssl_locks[n]); + } else { + git_mutex_unlock(&openssl_locks[n]); + } +} +#endif + + static void init_ssl(void) { #ifdef GIT_SSL SSL_load_error_strings(); OpenSSL_add_ssl_algorithms(); git__ssl_ctx = SSL_CTX_new(SSLv23_method()); + SSL_CTX_set_mode(git__ssl_ctx, SSL_MODE_AUTO_RETRY); + SSL_CTX_set_verify(git__ssl_ctx, SSL_VERIFY_NONE, NULL); + if (!SSL_CTX_set_default_verify_paths(git__ssl_ctx)) { + SSL_CTX_free(git__ssl_ctx); + git__ssl_ctx = NULL; + } + +# ifdef GIT_THREADS + { + int num_locks, i; + + num_locks = CRYPTO_num_locks(); + openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); + if (openssl_locks == NULL) { + SSL_CTX_free(git__ssl_ctx); + git__ssl_ctx = NULL; + } + + for (i = 0; i < num_locks; i++) { + if (git_mutex_init(&openssl_locks[i]) != 0) { + SSL_CTX_free(git__ssl_ctx); + git__ssl_ctx = NULL; + } + } + + CRYPTO_set_locking_callback(openssl_locking_function); + } +# endif #endif } @@ -183,8 +228,6 @@ static void init_once(void) { if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0) return; - if ((init_error = git_mutex_init(&git__ssl_mutex)) != 0) - return; pthread_key_create(&_tls_key, &cb__free_status); diff --git a/src/global.h b/src/global.h index 8904e2de5..745df3e4a 100644 --- a/src/global.h +++ b/src/global.h @@ -23,8 +23,6 @@ extern SSL_CTX *git__ssl_ctx; git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; -extern git_mutex git__ssl_mutex; -extern git_atomic git__ssl_init; #define GIT_GLOBAL (git__global_state()) diff --git a/src/netops.c b/src/netops.c index 54804d418..965e4775d 100644 --- a/src/netops.c +++ b/src/netops.c @@ -35,11 +35,6 @@ #include "http_parser.h" #include "global.h" -#if defined(GIT_SSL) && defined(GIT_THREADS) -/* OpenSSL wants us to keep an array of locks */ -static git_mutex *openssl_locks; -#endif - #ifdef GIT_WIN32 static void net_set_error(const char *str) { @@ -391,86 +386,14 @@ cert_fail_name: return -1; } -#ifdef GIT_THREADS -void openssl_locking_function(int mode, int n, const char *file, int line) -{ - int lock; - - GIT_UNUSED(file); - GIT_UNUSED(line); - - lock = mode & CRYPTO_LOCK; - - if (lock) { - git_mutex_lock(&openssl_locks[n]); - } else { - git_mutex_unlock(&openssl_locks[n]); - } -} -#endif - -/** - * The OpenSSL init functions are not reentrant so we need to init - * them under lock. - */ -static int init_ssl(void) -{ - if (git__ssl_init.val) - return 0; - - if (git_mutex_lock(&git__ssl_mutex) < 0) { - giterr_set(GITERR_OS, "failed to acquire ssl init lock"); - return -1; - } - - /* if we had to wait for the lock, someone else did it, we can return */ - if (git__ssl_init.val) - return 0; - - SSL_CTX_set_mode(git__ssl_ctx, SSL_MODE_AUTO_RETRY); - SSL_CTX_set_verify(git__ssl_ctx, SSL_VERIFY_NONE, NULL); - if (!SSL_CTX_set_default_verify_paths(git__ssl_ctx)) { - unsigned long err = ERR_get_error(); - giterr_set(GITERR_SSL, "failed to set verify paths: %s\n", ERR_error_string(err, NULL)); - return -1; - } - -#ifdef GIT_THREADS - { - int num_locks, i; - - num_locks = CRYPTO_num_locks(); - openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); - if (openssl_locks == NULL) { - git_mutex_unlock(&git__ssl_mutex); - return -1; - } - GITERR_CHECK_ALLOC(openssl_locks); - - for (i = 0; i < num_locks; i++) { - if (git_mutex_init(&openssl_locks[i]) != 0) { - git_mutex_unlock(&git__ssl_mutex); - giterr_set(GITERR_SSL, "failed to init lock %d", i); - return -1; - } - } - } - - CRYPTO_set_locking_callback(openssl_locking_function); -#endif - - git_atomic_inc(&git__ssl_init); - git_mutex_unlock(&git__ssl_mutex); - - return 0; -} - static int ssl_setup(gitno_socket *socket, const char *host, int flags) { int ret; - if (init_ssl() < 0) + if (git__ssl_ctx == NULL) { + giterr_set(GITERR_NET, "OpenSSL initialization failed"); return -1; + } socket->ssl.ssl = SSL_new(git__ssl_ctx); if (socket->ssl.ssl == NULL)