add locking around fake_now
authorNeil Horman <nhorman@openssl.org>
Fri, 3 Nov 2023 16:56:40 +0000 (12:56 -0400)
committerTomas Mraz <tomas@openssl.org>
Wed, 8 Nov 2023 14:55:40 +0000 (15:55 +0100)
fake_now in the quictestlib is read/written by potentially many threads,
and as such should have a surrounding lock to prevent WAR/RAW errors as
caught by tsan:

2023-11-03T16:27:23.7184999Z ==================
2023-11-03T16:27:23.7185290Z WARNING: ThreadSanitizer: data race (pid=18754)
2023-11-03T16:27:23.7185720Z   Read of size 8 at 0x558f6f9fe970 by main thread:
2023-11-03T16:27:23.7186726Z     #0 qtest_create_quic_connection_ex <null> (quicapitest+0x14aead) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7187665Z     #1 qtest_create_quic_connection <null> (quicapitest+0x14b220) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7188567Z     #2 test_quic_write_read quicapitest.c (quicapitest+0x150ee2) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7189561Z     #3 run_tests <null> (quicapitest+0x2237ab) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7190294Z     #4 main <null> (quicapitest+0x223d2b) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7190720Z
2023-11-03T16:27:23.7190902Z   Previous write of size 8 at 0x558f6f9fe970 by thread T1:
2023-11-03T16:27:23.7191607Z     #0 qtest_create_quic_connection_ex <null> (quicapitest+0x14aecf) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7192505Z     #1 run_server_thread quictestlib.c (quicapitest+0x14b1d6) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7193361Z     #2 thread_run quictestlib.c (quicapitest+0x14cadf) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7193848Z
2023-11-03T16:27:23.7194220Z   Location is global 'fake_now.0' of size 8 at 0x558f6f9fe970 (quicapitest+0x1af4970)
2023-11-03T16:27:23.7194636Z
2023-11-03T16:27:23.7194816Z   Thread T1 (tid=18760, running) created by main thread at:
2023-11-03T16:27:23.7195465Z     #0 pthread_create <null> (quicapitest+0xca12d) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7196317Z     #1 qtest_create_quic_connection_ex <null> (quicapitest+0x14adcb) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7197214Z     #2 qtest_create_quic_connection <null> (quicapitest+0x14b220) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7198111Z     #3 test_quic_write_read quicapitest.c (quicapitest+0x150ee2) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7198940Z     #4 run_tests <null> (quicapitest+0x2237ab) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7199661Z     #5 main <null> (quicapitest+0x223d2b) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5)
2023-11-03T16:27:23.7200083Z
2023-11-03T16:27:23.7200862Z SUMMARY: ThreadSanitizer: data race (/home/runner/work/openssl/openssl/test/quicapitest+0x14aead) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5) in qtest_create_quic_connection_ex

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/22616)

test/helpers/quictestlib.c

index 2c74b26252b4f5e1a11d3bef8408d01af1fc290e..0348729b726f52b05e58c5e351f053fb04b32d9d 100644 (file)
@@ -74,13 +74,16 @@ struct qtest_fault {
 
 static void packet_plain_finish(void *arg);
 static void handshake_finish(void *arg);
+static OSSL_TIME qtest_get_time(void);
+static void qtest_reset_time(void);
 
 static int using_fake_time = 0;
 static OSSL_TIME fake_now;
+static CRYPTO_RWLOCK *fake_now_lock = NULL;
 
 static OSSL_TIME fake_now_cb(void *arg)
 {
-    return fake_now;
+    return qtest_get_time();
 }
 
 static void noise_msg_callback(int write_p, int version, int content_type,
@@ -282,11 +285,14 @@ int qtest_create_quic_objects(OSSL_LIB_CTX *libctx, SSL_CTX *clientctx,
     if (serverctx != NULL && !TEST_true(SSL_CTX_up_ref(serverctx)))
         goto err;
     tserver_args.ctx = serverctx;
+    if (fake_now_lock == NULL) {
+        fake_now_lock = CRYPTO_THREAD_lock_new();
+        if (fake_now_lock == NULL)
+            goto err;
+    }
     if ((flags & QTEST_FLAG_FAKE_TIME) != 0) {
         using_fake_time = 1;
-        fake_now = ossl_time_zero();
-        /* zero time can have a special meaning, bump it */
-        qtest_add_time(1);
+        qtest_reset_time();
         tserver_args.now_cb = fake_now_cb;
         (void)ossl_quic_conn_set_override_now_cb(*cssl, fake_now_cb, NULL);
     } else {
@@ -331,7 +337,31 @@ int qtest_create_quic_objects(OSSL_LIB_CTX *libctx, SSL_CTX *clientctx,
 
 void qtest_add_time(uint64_t millis)
 {
+    if (!CRYPTO_THREAD_write_lock(fake_now_lock))
+        return;
     fake_now = ossl_time_add(fake_now, ossl_ms2time(millis));
+    CRYPTO_THREAD_unlock(fake_now_lock);
+}
+
+static OSSL_TIME qtest_get_time(void)
+{
+    OSSL_TIME ret;
+
+    if (!CRYPTO_THREAD_read_lock(fake_now_lock))
+        return ossl_time_zero();
+    ret = fake_now;
+    CRYPTO_THREAD_unlock(fake_now_lock);
+    return ret;
+}
+
+static void qtest_reset_time(void)
+{
+    if (!CRYPTO_THREAD_write_lock(fake_now_lock))
+        return;
+    fake_now = ossl_time_zero();
+    CRYPTO_THREAD_unlock(fake_now_lock);
+    /* zero time can have a special meaning, bump it */
+    qtest_add_time(1);
 }
 
 QTEST_FAULT *qtest_create_injector(QUIC_TSERVER *ts)
@@ -399,17 +429,20 @@ int qtest_wait_for_timeout(SSL *s, QUIC_TSERVER *qtserv)
      */
     if (!SSL_get_event_timeout(s, &tv, &cinf))
         return 0;
+
     if (using_fake_time)
-        now = fake_now;
+        now = qtest_get_time();
     else
         now = ossl_time_now();
+
     ctimeout = cinf ? ossl_time_infinite() : ossl_time_from_timeval(tv);
     stimeout = ossl_time_subtract(ossl_quic_tserver_get_deadline(qtserv), now);
     mintimeout = ossl_time_min(ctimeout, stimeout);
     if (ossl_time_is_infinite(mintimeout))
         return 0;
+
     if (using_fake_time)
-        fake_now = ossl_time_add(now, mintimeout);
+        qtest_add_time(ossl_time2ms(mintimeout));
     else
         OSSL_sleep(ossl_time2ms(mintimeout));