From 9ef9088c1585e13b9727796f15f77da64dbbe623 Mon Sep 17 00:00:00 2001 From: Andy Polyakov Date: Sun, 29 Jul 2018 14:12:53 +0200 Subject: [PATCH] ssl/*: switch to switch to Thread-Sanitizer-friendly primitives. Reviewed-by: Paul Dale Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/6786) --- ssl/ssl_lib.c | 41 +++++++++++++--------------------------- ssl/ssl_locl.h | 33 +++++++++++++++++--------------- ssl/ssl_sess.c | 20 +++++++------------- ssl/statem/extensions.c | 8 +++----- ssl/statem/statem_clnt.c | 4 +--- ssl/statem/statem_lib.c | 23 +++++++--------------- 6 files changed, 49 insertions(+), 80 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 15380e1b72..a486356c2a 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2264,7 +2264,6 @@ LHASH_OF(SSL_SESSION) *SSL_CTX_sessions(SSL_CTX *ctx) long SSL_CTX_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) { long l; - int i; /* For some cases with ctx == NULL perform syntax checks */ if (ctx == NULL) { switch (cmd) { @@ -2319,40 +2318,27 @@ long SSL_CTX_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) case SSL_CTRL_SESS_NUMBER: return lh_SSL_SESSION_num_items(ctx->sessions); case SSL_CTRL_SESS_CONNECT: - return CRYPTO_atomic_read(&ctx->stats.sess_connect, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_connect); case SSL_CTRL_SESS_CONNECT_GOOD: - return CRYPTO_atomic_read(&ctx->stats.sess_connect_good, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_connect_good); case SSL_CTRL_SESS_CONNECT_RENEGOTIATE: - return CRYPTO_atomic_read(&ctx->stats.sess_connect_renegotiate, &i, - ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_connect_renegotiate); case SSL_CTRL_SESS_ACCEPT: - return CRYPTO_atomic_read(&ctx->stats.sess_accept, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_accept); case SSL_CTRL_SESS_ACCEPT_GOOD: - return CRYPTO_atomic_read(&ctx->stats.sess_accept_good, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_accept_good); case SSL_CTRL_SESS_ACCEPT_RENEGOTIATE: - return CRYPTO_atomic_read(&ctx->stats.sess_accept_renegotiate, &i, - ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_accept_renegotiate); case SSL_CTRL_SESS_HIT: - return CRYPTO_atomic_read(&ctx->stats.sess_hit, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_hit); case SSL_CTRL_SESS_CB_HIT: - return CRYPTO_atomic_read(&ctx->stats.sess_cb_hit, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_cb_hit); case SSL_CTRL_SESS_MISSES: - return CRYPTO_atomic_read(&ctx->stats.sess_miss, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_miss); case SSL_CTRL_SESS_TIMEOUTS: - return CRYPTO_atomic_read(&ctx->stats.sess_timeout, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_timeout); case SSL_CTRL_SESS_CACHE_FULL: - return CRYPTO_atomic_read(&ctx->stats.sess_cache_full, &i, ctx->lock) - ? i : 0; + return tsan_load(&ctx->stats.sess_cache_full); case SSL_CTRL_MODE: return (ctx->mode |= larg); case SSL_CTRL_CLEAR_MODE: @@ -3426,13 +3412,12 @@ void ssl_update_cache(SSL *s, int mode) /* auto flush every 255 connections */ if ((!(i & SSL_SESS_CACHE_NO_AUTO_CLEAR)) && ((i & mode) == mode)) { - int *stat, val; + TSAN_QUALIFIER int *stat; if (mode & SSL_SESS_CACHE_CLIENT) stat = &s->session_ctx->stats.sess_connect_good; else stat = &s->session_ctx->stats.sess_accept_good; - if (CRYPTO_atomic_read(stat, &val, s->session_ctx->lock) - && (val & 0xff) == 0xff) + if ((tsan_load(stat) & 0xff) == 0xff) SSL_CTX_flush_sessions(s->session_ctx, (unsigned long)time(NULL)); } } diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index e7258d439a..a1a880c05c 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -33,6 +33,7 @@ # include "packet_locl.h" # include "internal/dane.h" # include "internal/refcount.h" +# include "internal/tsan_assist.h" # ifdef OPENSSL_BUILD_SHLIBSSL # undef OPENSSL_EXTERN @@ -779,21 +780,23 @@ struct ssl_ctx_st { const unsigned char *data, int len, int *copy); struct { - int sess_connect; /* SSL new conn - started */ - int sess_connect_renegotiate; /* SSL reneg - requested */ - int sess_connect_good; /* SSL new conne/reneg - finished */ - int sess_accept; /* SSL new accept - started */ - int sess_accept_renegotiate; /* SSL reneg - requested */ - int sess_accept_good; /* SSL accept/reneg - finished */ - int sess_miss; /* session lookup misses */ - int sess_timeout; /* reuse attempt on timeouted session */ - int sess_cache_full; /* session removed due to full cache */ - int sess_hit; /* session reuse actually done */ - int sess_cb_hit; /* session-id that was not in the cache was - * passed back via the callback. This - * indicates that the application is supplying - * session-id's from other processes - spooky - * :-) */ + TSAN_QUALIFIER int sess_connect; /* SSL new conn - started */ + TSAN_QUALIFIER int sess_connect_renegotiate; /* SSL reneg - requested */ + TSAN_QUALIFIER int sess_connect_good; /* SSL new conne/reneg - finished */ + TSAN_QUALIFIER int sess_accept; /* SSL new accept - started */ + TSAN_QUALIFIER int sess_accept_renegotiate; /* SSL reneg - requested */ + TSAN_QUALIFIER int sess_accept_good; /* SSL accept/reneg - finished */ + TSAN_QUALIFIER int sess_miss; /* session lookup misses */ + TSAN_QUALIFIER int sess_timeout; /* reuse attempt on timeouted session */ + TSAN_QUALIFIER int sess_cache_full; /* session removed due to full cache */ + TSAN_QUALIFIER int sess_hit; /* session reuse actually done */ + TSAN_QUALIFIER int sess_cb_hit; /* session-id that was not in + * the cache was passed back via + * the callback. This indicates + * that the application is + * supplying session-id's from + * other processes - spooky + * :-) */ } stats; CRYPTO_REF_COUNT references; diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index d4a4808f19..5ad2792a1b 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -448,7 +448,6 @@ SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id, size_t sess_id_len) { SSL_SESSION *ret = NULL; - int discard; if ((s->session_ctx->session_cache_mode & SSL_SESS_CACHE_NO_INTERNAL_LOOKUP) == 0) { @@ -469,8 +468,7 @@ SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id, } CRYPTO_THREAD_unlock(s->session_ctx->lock); if (ret == NULL) - CRYPTO_atomic_add(&s->session_ctx->stats.sess_miss, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_miss); } if (ret == NULL && s->session_ctx->get_session_cb != NULL) { @@ -479,8 +477,7 @@ SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id, ret = s->session_ctx->get_session_cb(s, sess_id, sess_id_len, ©); if (ret != NULL) { - CRYPTO_atomic_add(&s->session_ctx->stats.sess_cb_hit, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_cb_hit); /* * Increment reference count now if the session callback asks us @@ -533,7 +530,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) /* This is used only by servers. */ SSL_SESSION *ret = NULL; - int fatal = 0, discard; + int fatal = 0; int try_session_cache = 0; SSL_TICKET_STATUS r; @@ -612,8 +609,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) } if (ret->timeout < (long)(time(NULL) - ret->time)) { /* timeout */ - CRYPTO_atomic_add(&s->session_ctx->stats.sess_timeout, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_timeout); if (try_session_cache) { /* session was from the cache, so remove it */ SSL_CTX_remove_session(s->session_ctx, ret); @@ -641,8 +637,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) s->session = ret; } - CRYPTO_atomic_add(&s->session_ctx->stats.sess_hit, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_hit); s->verify_result = s->session->verify_result; return 1; @@ -669,7 +664,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *c) { - int ret = 0, discard; + int ret = 0; SSL_SESSION *s; /* @@ -736,8 +731,7 @@ int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *c) if (!remove_session_lock(ctx, ctx->session_cache_tail, 0)) break; else - CRYPTO_atomic_add(&ctx->stats.sess_cache_full, 1, &discard, - ctx->lock); + tsan_counter(&ctx->stats.sess_cache_full); } } } diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 85945acc0d..12c712e8e6 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -912,7 +912,7 @@ static int init_server_name(SSL *s, unsigned int context) static int final_server_name(SSL *s, unsigned int context, int sent) { - int ret = SSL_TLSEXT_ERR_NOACK, discard; + int ret = SSL_TLSEXT_ERR_NOACK; int altmp = SSL_AD_UNRECOGNIZED_NAME; int was_ticket = (SSL_get_options(s) & SSL_OP_NO_TICKET) == 0; @@ -960,10 +960,8 @@ static int final_server_name(SSL *s, unsigned int context, int sent) * exceed sess_accept (zero) for the new context. */ if (SSL_IS_FIRST_HANDSHAKE(s) && s->ctx != s->session_ctx) { - CRYPTO_atomic_add(&s->ctx->stats.sess_accept, 1, &discard, - s->ctx->lock); - CRYPTO_atomic_add(&s->session_ctx->stats.sess_accept, -1, &discard, - s->session_ctx->lock); + tsan_counter(&s->ctx->stats.sess_accept); + tsan_counter(&s->session_ctx->stats.sess_accept); } /* diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index e846f772a6..3dc29cc757 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1409,7 +1409,6 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) unsigned int compression; unsigned int sversion; unsigned int context; - int discard; RAW_EXTENSION *extensions = NULL; #ifndef OPENSSL_NO_COMP SSL_COMP *comp; @@ -1616,8 +1615,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) || (SSL_IS_TLS13(s) && s->session->ext.tick_identity != TLSEXT_PSK_BAD_IDENTITY)) { - CRYPTO_atomic_add(&s->session_ctx->stats.sess_miss, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_miss); if (!ssl_get_new_session(s, 0)) { /* SSLfatal() already called */ goto err; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index ebb21deb8b..caed61aaac 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -132,23 +132,18 @@ int tls_setup_handshake(SSL *s) } if (SSL_IS_FIRST_HANDSHAKE(s)) { /* N.B. s->session_ctx == s->ctx here */ - CRYPTO_atomic_add(&s->session_ctx->stats.sess_accept, 1, &i, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_accept); } else { /* N.B. s->ctx may not equal s->session_ctx */ - CRYPTO_atomic_add(&s->ctx->stats.sess_accept_renegotiate, 1, &i, - s->ctx->lock); + tsan_counter(&s->ctx->stats.sess_accept_renegotiate); s->s3->tmp.cert_request = 0; } } else { - int discard; if (SSL_IS_FIRST_HANDSHAKE(s)) - CRYPTO_atomic_add(&s->session_ctx->stats.sess_connect, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_connect); else - CRYPTO_atomic_add(&s->session_ctx->stats.sess_connect_renegotiate, - 1, &discard, s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_connect_renegotiate); /* mark client_random uninitialized */ memset(s->s3->client_random, 0, sizeof(s->s3->client_random)); @@ -1009,7 +1004,6 @@ unsigned long ssl3_output_cert_chain(SSL *s, WPACKET *pkt, CERT_PKEY *cpk) */ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) { - int discard; void (*cb) (const SSL *ssl, int type, int val) = NULL; if (clearbufs) { @@ -1055,8 +1049,7 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) ssl_update_cache(s, SSL_SESS_CACHE_SERVER); /* N.B. s->ctx may not equal s->session_ctx */ - CRYPTO_atomic_add(&s->ctx->stats.sess_accept_good, 1, &discard, - s->ctx->lock); + tsan_counter(&s->ctx->stats.sess_accept_good); s->handshake_func = ossl_statem_accept; if (SSL_IS_DTLS(s) && !s->hit) { @@ -1084,12 +1077,10 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) ssl_update_cache(s, SSL_SESS_CACHE_CLIENT); } if (s->hit) - CRYPTO_atomic_add(&s->session_ctx->stats.sess_hit, 1, &discard, - s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_hit); s->handshake_func = ossl_statem_connect; - CRYPTO_atomic_add(&s->session_ctx->stats.sess_connect_good, 1, - &discard, s->session_ctx->lock); + tsan_counter(&s->session_ctx->stats.sess_connect_good); if (SSL_IS_DTLS(s) && s->hit) { /* -- 2.34.1