tests: reset freed pointers in mem-fail TLS tests to avoid double-free - #11398
Open
sameehj wants to merge 1 commit into
Open
tests: reset freed pointers in mem-fail TLS tests to avoid double-free#11398sameehj wants to merge 1 commit into
sameehj wants to merge 1 commit into
Conversation
Under WOLFSSL_MEM_FAIL_COUNT injection, once an Expect* assertion fails it latches the test result and turns every subsequent Expect* into a no-op - including the pointer-assigning ones. Multi-block tests that reuse WOLFSSL/WOLFSSL_CTX (and other SSL-owned objects) across blocks then skip the later block's ExpectNotNull(ptr = ...) assignment, leaving a stale, already-freed pointer that the unconditional cleanup frees again. This double-free/use-after-free surfaces as an intermittent SIGSEGV/abort in the nightly mem-fail test. Reset ssl/ctx to NULL after each wolfSSL_free()/wolfSSL_CTX_free() in the affected multi-block tests so a skipped re-assignment leaves NULL and the final free is a no-op. Also clear other SSL-owned handles (the ECH struct, pre-shared keys and TLSX extension pointers) that are dereferenced after the owning SSL is freed, and make test_tls_msgtype_sni_parse_ech_public free its ech when TLSX_Push() fails instead of leaking it. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11398
Scan targets checked: wolfssl-bugs, wolfssl-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Contributor
Author
|
retest this please |
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
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.
Description
This is a test-only change. It does not change library code or run-time behavior.
The nightly mem-fail job builds with
WOLFSSL_MEM_FAIL_COUNTand makes each memory allocation fail in turn. When an allocation fails, anExpect*assertion fails and latches the test result. After this point, the test framework does not evaluate any moreExpect*statements. It therefore skips the pointer assignment inside a laterExpectNotNull(ptr = wolfSSL_new(...)).Several unit tests run more than one setup/free block against the same
sslandctxvariables. If the framework skips the re-assignment in a later block, the variable still holds the pointer that the previous block freed. The unconditional cleanup at the end of the block then frees that pointer a second time. The result is a double-free or use-after-free. Because the failure depends on heap state, it appears as an intermittent SIGSEGV (139) or abort (134), not a stable failure.The fix sets each pointer to
NULLimmediately after the test frees it. A later skipped assignment then leavesNULL, andwolfSSL_free(NULL)/wolfSSL_CTX_free(NULL)do nothing. Three tests also hold objects that theWOLFSSLowns (an ECH struct, pre-shared keys, and TLSX extension pointers) and dereference them in a later block through a bareif (ptr != NULL)guard. These handles are cleared for the same reason. Intest_tls_msgtype_sni_parse_ech_public, a failedTLSX_Push()does not transfer ownership ofech(TLSX_New()returnsNULLwithout storingdata), so the test now freesechon that path instead of leaking it.The change applies the same pattern that
test_ProcessChainOCSPRequest_boundsalready uses in this file. Two multi-block tests (test_wolfSSL_SetTlsHmacInner_boundsandtest_ProcessChainOCSPRequest_bounds) already reset their pointers and are left unchanged.Affected tests:
test_tls_msgtype.c:certificate_ext_offered,sni_parse_match,sni_parse_ech_public,psk_ch_id_gates,psk_sh_index,psk_sh_resumption,psk_ch_binder_gates,cookie_parse_gates,tca_parse_gates,tca_find,tca_new_alloc,psk_write_chosen.test_tls_bounds.c:test_TLSX_PopulateExtensions_bounds,test_TLSX_ext_msgtype_dispatch_bounds,test_TLSX_CSR_Parse_bounds,test_TLSX_ALPN_GetSize_overflow,test_TLSX_PopulateSupportedGroups_bounds,test_TLSX_SecureRenegotiation_Write_bounds.Note: the crashes are not deterministic, so a single scan can miss one. For this reason the change hardens all multi-block tests in these two files, not only the tests that crashed in one run.
Fixes zd#
Testing
Build:
Method: for each affected test, read its baseline allocation count, then run the test once per injection point (
MEM_FAIL_CNT=1..MAX). Repeat each scan several times, because the failure is not deterministic. Record the exit code and compareMemFailCount TotalagainstMemFailCount Frees.Results:
test_TLSX_CSR_Parse_bounds; a repeated scan then showed it crashes atMEM_FAIL_CNT=137.Dependency: the same nightly job also reports two
Free/Allocmismatches (test_tls_msgtype_tca_new_allocat 22 andtest_ProcessChainOCSPRequest_boundsat 137). These are memory-count and leak problems, not crashes, and PR #11397 fixes them. The nightly job is fully clean only when both PR #11397 and this PR are present.Checklist