Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Correct order of ossl_condvar_signal in quic_multistream_test #22616

Closed
wants to merge 7 commits into from
Closed

Correct order of ossl_condvar_signal in quic_multistream_test #22616

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecad973
Correct order of ossl_condvar_signal in quic_multistream_test
nhorman Nov 3, 2023
a3c1041
add locking around fake_now
nhorman Nov 3, 2023
c6398a9
fixup! add locking around fake_now
nhorman Nov 3, 2023
5b659c1
fixup! add locking around fake_now
nhorman Nov 6, 2023
620c64d
fixup! Correct order of ossl_condvar_signal in quic_multistream_test
nhorman Nov 6, 2023
c575519
fixup! add locking around fake_now
nhorman Nov 7, 2023
4f32438
fixup! add locking around fake_now
nhorman Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 42 additions & 10 deletions test/helpers/quictestlib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
nhorman marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand Down Expand Up @@ -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_read_lock(fake_now_lock))
nhorman marked this conversation as resolved.
Show resolved Hide resolved
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);
Comment on lines +363 to +364
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this racy? Could you just ossl_time_add() inside the write lock?

Alternatively you can see this function as something that should be never used in racy contexts and drop the lock calls completely here. Using this in a racy context would mean that we need to have multiple fake_now storages per use actually because otherwise one thread could reset the fake time for another innocent thread and the time would go backwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is racy. qtest_add_time retakes the global lock, so there aren't any read/write races strictly speaking. It is theoretically possible that another thread might read the zero time, but given that this is during setup, there should be no other threads running

}

QTEST_FAULT *qtest_create_injector(QUIC_TSERVER *ts)
Expand Down Expand Up @@ -399,19 +429,21 @@ 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;
else
if (using_fake_time) {
nhorman marked this conversation as resolved.
Show resolved Hide resolved
now = qtest_get_time();
} else {
now = ossl_time_now();
}
nhorman marked this conversation as resolved.
Show resolved Hide resolved
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);
else
if (using_fake_time) {
qtest_add_time(ossl_time2ms(mintimeout));
} else {
OSSL_sleep(ossl_time2ms(mintimeout));
}
nhorman marked this conversation as resolved.
Show resolved Hide resolved

return 1;
}
Expand Down
4 changes: 2 additions & 2 deletions test/quic_multistream_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,8 @@ static int join_server_thread(struct helper *h)

ossl_crypto_mutex_lock(h->server_thread.m);
h->server_thread.stop = 1;
ossl_crypto_mutex_unlock(h->server_thread.m);
ossl_crypto_condvar_signal(h->server_thread.c);
ossl_crypto_mutex_unlock(h->server_thread.m);

ossl_crypto_thread_native_join(h->server_thread.t, &rv);
ossl_crypto_thread_native_clean(h->server_thread.t);
Expand Down Expand Up @@ -1079,8 +1079,8 @@ static int run_script_worker(struct helper *h, const struct script_op *script,
else if (h->blocking && !h->server_thread.ready) {
ossl_crypto_mutex_lock(h->server_thread.m);
h->server_thread.ready = 1;
ossl_crypto_mutex_unlock(h->server_thread.m);
ossl_crypto_condvar_signal(h->server_thread.c);
ossl_crypto_mutex_unlock(h->server_thread.m);
}
if (h->blocking)
assert(h->s == NULL);
Expand Down