From d025b228fe4c05d4307d245cc888d881e5555858 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Fri, 3 Nov 2023 12:56:40 -0400 Subject: [PATCH] add locking around fake_now 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 (quicapitest+0x14aead) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5) 2023-11-03T16:27:23.7187665Z #1 qtest_create_quic_connection (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 (quicapitest+0x2237ab) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5) 2023-11-03T16:27:23.7190294Z #4 main (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 (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 (quicapitest+0xca12d) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5) 2023-11-03T16:27:23.7196317Z #1 qtest_create_quic_connection_ex (quicapitest+0x14adcb) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5) 2023-11-03T16:27:23.7197214Z #2 qtest_create_quic_connection (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 (quicapitest+0x2237ab) (BuildId: d06f7b04830b55de9c8482b398a1781472d1c7d5) 2023-11-03T16:27:23.7199661Z #5 main (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 Reviewed-by: Paul Dale Reviewed-by: Matt Caswell Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/22616) (cherry picked from commit 11179b3e8de8cd566af1215093db793ac3ed0f91) --- test/helpers/quictestlib.c | 45 +++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/test/helpers/quictestlib.c b/test/helpers/quictestlib.c index 2c74b26252b4f..0348729b726f5 100644 --- a/test/helpers/quictestlib.c +++ b/test/helpers/quictestlib.c @@ -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));