Use atomics for SSL_CTX statistics

It is expected that SSL_CTX objects are shared across threads,
and as such we are responsible for ensuring coherent data accesses.
Aligned integer accesses ought to be atomic already on all supported
architectures, but we can be formally correct.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/4549)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 4435efdb0ccf370620ce18ee3f9256542d6a2223..c151e7e27e17437791e7085614b6b01840137b0d 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2180,6 +2180,7 @@ 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) {
@@ -2234,27 +2235,40 @@ 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 ctx->stats.sess_connect;
+        return CRYPTO_atomic_read(&ctx->stats.sess_connect, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_CONNECT_GOOD:
-        return ctx->stats.sess_connect_good;
+        return CRYPTO_atomic_read(&ctx->stats.sess_connect_good, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_CONNECT_RENEGOTIATE:
-        return ctx->stats.sess_connect_renegotiate;
+        return CRYPTO_atomic_read(&ctx->stats.sess_connect_renegotiate, &i,
+                                  ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_ACCEPT:
-        return ctx->stats.sess_accept;
+        return CRYPTO_atomic_read(&ctx->stats.sess_accept, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_ACCEPT_GOOD:
-        return ctx->stats.sess_accept_good;
+        return CRYPTO_atomic_read(&ctx->stats.sess_accept_good, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_ACCEPT_RENEGOTIATE:
-        return ctx->stats.sess_accept_renegotiate;
+        return CRYPTO_atomic_read(&ctx->stats.sess_accept_renegotiate, &i,
+                                  ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_HIT:
-        return ctx->stats.sess_hit;
+        return CRYPTO_atomic_read(&ctx->stats.sess_hit, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_CB_HIT:
-        return ctx->stats.sess_cb_hit;
+        return CRYPTO_atomic_read(&ctx->stats.sess_cb_hit, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_MISSES:
-        return ctx->stats.sess_miss;
+        return CRYPTO_atomic_read(&ctx->stats.sess_miss, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_TIMEOUTS:
-        return ctx->stats.sess_timeout;
+        return CRYPTO_atomic_read(&ctx->stats.sess_timeout, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_SESS_CACHE_FULL:
-        return ctx->stats.sess_cache_full;
+        return CRYPTO_atomic_read(&ctx->stats.sess_cache_full, &i, ctx->lock)
+                ? i : 0;
     case SSL_CTRL_MODE:
         return (ctx->mode |= larg);
     case SSL_CTRL_CLEAR_MODE:
@@ -3205,11 +3219,14 @@ void ssl_update_cache(SSL *s, int mode)
 
     /* auto flush every 255 connections */
     if ((!(i & SSL_SESS_CACHE_NO_AUTO_CLEAR)) && ((i & mode) == mode)) {
-        if ((((mode & SSL_SESS_CACHE_CLIENT)
-              ? s->session_ctx->stats.sess_connect_good
-              : s->session_ctx->stats.sess_accept_good) & 0xff) == 0xff) {
+        int *stat, val;
+        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)
             SSL_CTX_flush_sessions(s->session_ctx, (unsigned long)time(NULL));
-        }
     }
 }
 

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 9f5b016e3861c25c98b9fcf0ef354b87f7324811..c8d1cc37e5521adfb7a93659a2fb01d6e9e7d2b8 100644 (file)
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -461,7 +461,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al)
     /* This is used only by servers. */
 
     SSL_SESSION *ret = NULL;
-    int fatal = 0;
+    int fatal = 0, discard;
     int try_session_cache = 0;
     TICKET_RETURN r;
 
@@ -512,7 +512,8 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al)
         }
         CRYPTO_THREAD_unlock(s->session_ctx->lock);
         if (ret == NULL)
-            s->session_ctx->stats.sess_miss++;
+            CRYPTO_atomic_add(&s->session_ctx->stats.sess_miss, 1, &discard,
+                              s->session_ctx->lock);
     }
 
     if (try_session_cache &&
@@ -524,7 +525,8 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al)
                                              &copy);
 
         if (ret != NULL) {
-            s->session_ctx->stats.sess_cb_hit++;
+            CRYPTO_atomic_add(&s->session_ctx->stats.sess_cb_hit, 1, &discard,
+                              s->session_ctx->lock);
 
             /*
              * Increment reference count now if the session callback asks us
@@ -589,7 +591,8 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al)
     }
 
     if (ret->timeout < (long)(time(NULL) - ret->time)) { /* timeout */
-        s->session_ctx->stats.sess_timeout++;
+        CRYPTO_atomic_add(&s->session_ctx->stats.sess_timeout, 1, &discard,
+                          s->session_ctx->lock);
         if (try_session_cache) {
             /* session was from the cache, so remove it */
             SSL_CTX_remove_session(s->session_ctx, ret);
@@ -617,7 +620,8 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al)
         s->session = ret;
     }
 
-    s->session_ctx->stats.sess_hit++;
+    CRYPTO_atomic_add(&s->session_ctx->stats.sess_hit, 1, &discard,
+                      s->session_ctx->lock);
     s->verify_result = s->session->verify_result;
     return 1;
 
@@ -646,7 +650,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello, int *al)
 
 int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *c)
 {
-    int ret = 0;
+    int ret = 0, discard;
     SSL_SESSION *s;
 
     /*
@@ -713,7 +717,8 @@ int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *c)
                 if (!remove_session_lock(ctx, ctx->session_cache_tail, 0))
                     break;
                 else
-                    ctx->stats.sess_cache_full++;
+                    CRYPTO_atomic_add(&ctx->stats.sess_cache_full, 1, &discard,
+                                      ctx->lock);
             }
         }
     }

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index af42bcb0f361eaa03870904ff97651f051ae63ef..6b1bc9270006f3c79b3b7fbca97740ee4a795757 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1266,7 +1266,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
     unsigned int compression;
     unsigned int sversion;
     unsigned int context;
-    int protverr;
+    int protverr, discard;
     RAW_EXTENSION *extensions = NULL;
 #ifndef OPENSSL_NO_COMP
     SSL_COMP *comp;
@@ -1430,7 +1430,8 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
                 || (SSL_IS_TLS13(s)
                     && s->session->ext.tick_identity
                        != TLSEXT_PSK_BAD_IDENTITY)) {
-            s->ctx->stats.sess_miss++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_miss, 1, &discard,
+                              s->ctx->lock);
             if (!ssl_get_new_session(s, 0)) {
                 goto f_err;
             }

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index e36f98a8b4cb79915605659764dce1d4b5884340..bff3aa74021706896f0eec2e3df84968894232b2 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -111,7 +111,7 @@ int tls_setup_handshake(SSL *s)
             return 0;
         }
         if (SSL_IS_FIRST_HANDSHAKE(s)) {
-            s->ctx->stats.sess_accept++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_accept, 1, &i, s->ctx->lock);
         } else if ((s->options & SSL_OP_NO_RENEGOTIATION)) {
             /* Renegotiation is disabled */
             ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
@@ -128,15 +128,19 @@ int tls_setup_handshake(SSL *s)
             ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
             return 0;
         } else {
-            s->ctx->stats.sess_accept_renegotiate++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_accept_renegotiate, 1, &i,
+                              s->ctx->lock);
 
             s->s3->tmp.cert_request = 0;
         }
     } else {
+        int discard;
         if (SSL_IS_FIRST_HANDSHAKE(s))
-            s->ctx->stats.sess_connect++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_connect, 1, &discard,
+                              s->ctx->lock);
         else
-            s->ctx->stats.sess_connect_renegotiate++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_connect_renegotiate, 1,
+                              &discard, s->ctx->lock);
 
         /* mark client_random uninitialized */
         memset(s->s3->client_random, 0, sizeof(s->s3->client_random));
@@ -991,6 +995,7 @@ 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 discard;
     void (*cb) (const SSL *ssl, int type, int val) = NULL;
 
 #ifndef OPENSSL_NO_SCTP
@@ -1027,7 +1032,8 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs)
         if (s->server) {
             ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
 
-            s->ctx->stats.sess_accept_good++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_accept_good, 1, &discard,
+                              s->ctx->lock);
             s->handshake_func = ossl_statem_accept;
         } else {
             /*
@@ -1037,10 +1043,12 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs)
             if (!SSL_IS_TLS13(s))
                 ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
             if (s->hit)
-                s->ctx->stats.sess_hit++;
+                CRYPTO_atomic_add(&s->ctx->stats.sess_hit, 1, &discard,
+                                  s->ctx->lock);
 
             s->handshake_func = ossl_statem_connect;
-            s->ctx->stats.sess_connect_good++;
+            CRYPTO_atomic_add(&s->ctx->stats.sess_connect_good, 1, &discard,
+                              s->ctx->lock);
         }
 
         if (s->info_callback != NULL)