Bind test listeners before spawning the responder thread - #13
Open
Frauschi wants to merge 1 commit into
Open
Conversation
The mock HTTP responders in these tests created their listening socket inside the worker thread and handed the ephemeral port back through a plain int in a shared context struct, which the main thread then polled in a nanosleep loop. That handoff has no synchronisation, so it is a data race. ThreadSanitizer flags it and, with halt_on_error=1, aborts the run. The nightly Sanitizers workflow started failing on the scep_roundtrip case because the pattern was copied into an integration test, and the TSAN job runs the roundtrip tests. The unit tests carrying the same pattern were never TSAN-exercised, so they hid the same defect. Create the listening socket on the caller's thread instead and pass the descriptor into the responder. The port is known before pthread_create, so the shared field, the poll loop and the race all go away, and the tests no longer pay the 5 ms polling ticks. The new listen_loopback helper also checks the bind, listen and getsockname return codes, which the inline versions did not. test_tls_http.c already guarded its port with WOLFSSL_ATOMIC_LOAD and is left alone.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #13
Scan targets checked: wolfcert-bugs, wolfcert-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The mock HTTP responder threads in three tests created their listening socket inside the worker thread and handed the ephemeral port back to the main thread through a plain
intin a shared context struct. The main thread picked it up by polling that field in ananosleeploop.That handoff has no synchronisation, so it is a data race. ThreadSanitizer reports it and, with
halt_on_error=1, aborts the run:This is what has been failing the nightly Sanitizers workflow on main since 2026-07-25. Only the
ThreadSanitizer (roundtrips)job is affected; ASan/UBSan and valgrind cannot see data races and stayed green.Why it surfaced now
The idiom has been in
tests/unit/test_http.cfor a long time, but the TSAN job only runs-R 'roundtrip|tls_http', so the unit tests carrying it were never TSAN-exercised.7ac927acopied the pattern intotests/integration/test_scep_roundtrip.c, which does run under TSAN, and the latent defect became a red nightly. The unit tests were racy the whole time.How
Create the listening socket on the caller's thread and pass the descriptor into the responder through the context struct. The port is then known before
pthread_create, so the shared field, the poll loop and the race all go away. As a side effect the tests no longer pay up to 200 x 5 ms of polling ticks.The new
listen_loopback()helper also checks thebind,listenandgetsocknamereturn codes. The inline versions intest_est.cignored all three.tests/unit/test_http.csrv_thread,srv_retry_thread,srv_thread_overflow,srv_thread_capture)tests/unit/test_est.ctests/integration/test_scep_roundtrip.cNet -140 / +105. The now-unused
<time.h>includes are dropped.Scope
A sweep of every
nanosleepintests/confirms nothing else is affected:tests/integration/test_tls_http.calready guards its port withWOLFSSL_ATOMIC_LOAD. Correct as it stands, left alone.tests/integration/test_est_chunked_robustness.cis an unrelated TCP-segmentation helper, not a port handoff.No library code changes. Tests only.