tests: fix mem-fail crashes and the key-share leak by sameehj · Pull Request #11608 · wolfSSL/wolfssl

sameehj · 2026-09-30T13:54:03Z

Summary

Return from the DTLS API tests when wolfSSL_CTX_new or wolfSSL_new fails, so a later call does not use a null object.
Clear ssl and ctx after each free in the TLS extension tests that segfaulted when the next wolfSSL_CTX_new failed under MEM_FAIL_CNT.
When TLSX_Push cannot allocate the key-share list node, free the entry with the function for its group (FFDHE, X25519, X448, or ECC) instead of leaking it or calling wc_ecc_free on the wrong key.

Test plan

Nightly configure (--enable-all, -DWOLFSSL_MEM_FAIL_COUNT) and the nine failing indexes: exit 1, free count equal to allocation count, no segmentation fault.
MEM_FAIL_CNT on an X448 key share whose TLSX_Push is the failed allocation: no segmentation fault, free count equal to allocation count.
Confirm the tests that were not in the nightly errors are unchanged apart from the shared free helper.

github-actions · 2026-09-30T13:57:38Z

Copilot

Copilot review overview

🟡 Changes recommended

The key-share failure cleanup can still leak nonblocking contexts, and its X448 path remains untested.

Review effort: Balanced
Findings: 3 · 1

Open (4)

Leak FFDHE nonblocking buffer on TLSX_Push failure · New
Leak X25519 nonblocking context on TLSX_Push failure · New
Leak ECC nonblocking context on TLSX_Push failure · New
Add fault-injection coverage for failed key-share cleanup · New

What changed in this PR

This PR hardens TLS and DTLS API tests against allocation failures and adds cleanup for key-share entries when an extension-list allocation fails.

Changes:

Stop DTLS tests from using objects whose constructors failed, and clear freed TLS test pointers.
Free key-share entries by group when TLSX_Push fails.

File	Description
`tests/api/test_tls13.c`	Clears freed TLS test pointers.
`tests/api/test_tls_parse.c`	Clears freed pointers and adds key-share failure cleanup.
`tests/api/test_ssl_cert.c`	Returns early when DTLS test constructors fail.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

sameehj · 2026-10-01T08:42:13Z

Reply to the Copilot review.

The three nonblocking leaks are handled in a8f40dc. On a failed TLSX_Push, the FFDHE path frees DhKey.nb before wc_FreeDhKey, the X25519 path frees nb_ctx before wc_curve25519_free, and the ECC path frees nb_ctx before wc_ecc_free. Each free is under the same macros as TLSX_KeyShare_FreeAll. The private-key wipe calls wc_ForceZero, which is the symbol this test file can link.

The missing coverage is also in a8f40dc. test_TLSX_KeyShare_gen generates an FFDHE 2048, X25519, X448, and secp256r1 key, fails the next allocation (the list node), and frees that entry.

15a0f58 puts that helper under USE_WOLFSSL_MEMORY. wolfSSL_GetAllocators and wolfSSL_SetAllocators are built only with that macro, and White-box smoke reported the unguarded calls.

The WOLFSSL_TRACK_MEMORY make check failure is a 30 minute timeout of all-noasm-wolfentropy. That log shows 0 failed tests. The sibling all-wolfentropy config in the same job also ran far past its budget. Those timeouts are not an assertion failure from this change.

MemBrowse measures IDE/GCC-ARM WolfCryptTest. This pull request only changes tests/api, and that image does not compile those files. The report on 4140c96 found no size change against the same base.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #11608

Scan targets checked: wolfssl-bugs
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/api/test_ssl_cert.c

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #11608

Scan targets checked: wolfssl-bugs
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

philljj · 2026-10-06T17:34:54Z

+            kse->group = WOLFSSL_FFDHE_2048;
+            ExpectIntEQ(TLSX_KeyShare_GenKey(ssl, kse), 0);
+            ExpectNotNull(kse->pubKey);
+            ExpectIntEQ(test_tls_parse_free_kse_push_fail(ssl, kse), 0);


This test_tls_parse_free_kse_push_fail() is inside an Expect*() wrapper.

If kse is allocated, but an error happens later, then kse leaks.

Fixed in 60ae2f9. The helper now runs before ExpectIntEQ. A failed GenKey or pubKey check still frees kse. The X25519, X448, and P-256 blocks use the same order.

philljj · 2026-10-06T17:43:52Z

+            kse->group = WOLFSSL_ECC_X25519;
+            ExpectIntEQ(TLSX_KeyShare_GenKey(ssl, kse), 0);
+            ExpectNotNull(kse->pubKey);
+            ExpectIntEQ(test_tls_parse_free_kse_push_fail(ssl, kse), 0);


same, kse will leak when behind an Expect.*() gate on err.

Fixed in 60ae2f9. This block calls the helper, then ExpectIntEQ checks the return value.

philljj · 2026-10-06T17:47:09Z

+                    ssl->heap, DYNAMIC_TYPE_TLSX));
+        if (kse != NULL) {
+            XMEMSET(kse, 0, sizeof(*kse));
+            kse->group = WOLFSSL_ECC_SECP256R1;


WOLFSSL_ECC_SECP256R1 is usually behind a combined define gate like

#ifdef HAVE_ECC #if !defined(NO_ECC256) || defined(HAVE_ALL_CURVES) #ifndef NO_ECC_SECP case WOLFSSL_ECC_SECP256R1:

Fixed in 60ae2f9. This block is now under HAVE_ECC, HAVE_ECC_KEY_EXPORT, !NO_ECC256 or HAVE_ALL_CURVES, !NO_ECC_SECP, and ECC_MIN_KEY_SZ <= 256. That matches the P-256 arm in TLSX_KeyShare_GenEccKey. The null-rng case in the same block uses that gate too.

philljj · 2026-10-07T15:05:09Z

Retest this please

(apple-m1 test was still disrupted).

ExpectNotNull skips the following wolfSSL_new when wolfSSL_CTX_new fails, so wolfSSL_free ran on the previous SSL. Clear both pointers after each free. Skip the DTLS guard calls when the constructor did not produce an object. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

Once MEM_FAIL_CNT is hit, every later allocation fails. TLSX_Push then cannot take the peer entry, so test_TLSX_KeyShare_process leaked it. Free that entry directly. Clear the ServerHello test pointers after the first free so the next wolfSSL_free does not run twice. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

The push-failure path called wc_ecc_free for every group other than X25519, so a live X448 key was freed as an ECC key. FFDHE, X25519, X448, and ECC now each use their own free. Drop the pointer clears in tests that were not failing under MEM_FAIL_CNT. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

Call wc_ForceZero so this file builds where ForceZero is not in scope. Release the software-async buffers the same way TLSX_KeyShare_FreeAll does, and fail the list-node allocation for each generated key type. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

wolfSSL_GetAllocators and wolfSSL_SetAllocators are built only when USE_WOLFSSL_MEMORY is set. The new helper called them without that guard, so the API guard check failed. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

After an Expect fails, the next ExpectNotNull does not assign kse, so the push-fail block still named the entry just freed. Clear kse first. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

ExpectIntEQ does not call its argument after a failure, so the push-fail helper never freed the entry. Call it first, then check the result. Run the P-256 cases only when GenEccKey accepts that group. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

Copilot

🟢 Approval recommended

The changes consistently handle allocation failures and provide targeted coverage for the corrected cleanup paths.

0 open findings

4 resolved since last review

Leak ECC nonblocking context on TLSX_Push failure
Leak X25519 nonblocking context on TLSX_Push failure
Leak FFDHE nonblocking buffer on TLSX_Push failure
Add fault-injection coverage for failed key-share cleanup

🧠 Review effort: Balanced

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #11608

Scan targets checked: wolfssl-bugs
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Copilot AI balanced review requested due to automatic review settings September 30, 2026 13:54

Copilot started reviewing on behalf of sameehj September 30, 2026 13:54 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Comment thread tests/api/test_tls_parse.c Outdated

Comment thread tests/api/test_tls_parse.c Outdated

Comment thread tests/api/test_tls_parse.c Outdated

Comment thread tests/api/test_tls_parse.c

sameehj requested a review from wolfSSL-Fenrir-bot October 1, 2026 09:50

wolfSSL-Fenrir-bot previously requested changes Oct 1, 2026

View reviewed changes

Comment thread tests/api/test_tls_parse.c

sameehj assigned wolfSSL-Bot Oct 1, 2026

JacobBarthelmeh requested a review from wolfSSL-Fenrir-bot October 1, 2026 20:26

wolfSSL-Fenrir-bot reviewed Oct 1, 2026

View reviewed changes

philljj self-assigned this Oct 2, 2026

philljj requested changes Oct 6, 2026

View reviewed changes

philljj assigned sameehj and unassigned wolfSSL-Bot Oct 6, 2026

sameehj added 7 commits October 8, 2026 09:09

tests: drop the freed key-share pointer before the next case

59e3b14

After an Expect fails, the next ExpectNotNull does not assign kse, so the push-fail block still named the entry just freed. Clear kse first. Signed-off-by: Sameeh Jubran <sameeh@wolfssl.com>

sameehj force-pushed the fix/mem-fail-null-deref branch from 60ae2f9 to 41588f0 Compare October 8, 2026 06:10

sameehj requested review from philljj and wolfSSL-Fenrir-bot and a balanced review from Copilot October 8, 2026 06:24

sameehj assigned wolfSSL-Bot and unassigned sameehj Oct 9, 2026

Copilot started reviewing on behalf of sameehj October 9, 2026 06:04 View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

wolfSSL-Fenrir-bot reviewed Oct 9, 2026

View reviewed changes

Conversation

sameehj commented Sep 30, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sameehj commented Oct 1, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #11608

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #11608

Uh oh!

philljj Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

sameehj Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

philljj Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

sameehj Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

philljj Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

sameehj Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

philljj commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #11608

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Sep 30, 2026 •

edited

Loading

philljj commented Oct 7, 2026 •

edited

Loading