Skip to content

Fenrir 15782: wolfSSL Static blocks are ref counted and freed by last ref - #11899

Open
aidankeefe2022 wants to merge 3 commits into
wolfSSL:masterfrom
aidankeefe2022:ssl-sess-fenrir
Open

aidankeefe2022 wants to merge 3 commits into
wolfSSL:masterfrom
aidankeefe2022:ssl-sess-fenrir

Conversation

@aidankeefe2022

@aidankeefe2022 aidankeefe2022 commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Description

With WOLFSSL_STATIC_MEMORY, each SSL gets its own heap hint, and wolfSSL_free() used to release it. Some objects allocated from that hint can outlive the SSL: a session from wolfSSL_get1_session(), the server's peer cert stored in the session, or an X509 from wolfSSL_get_peer_certificate(). After wolfSSL_free(), freeing or using these objects touched a heap hint that had already been freed.

  • The per-SSL heap hint now has a reference count. The SSL holds one reference, and so does every block allocated from the hint.
  • Each allocated block stores its owning hint in wc_Memory.next, a field that sits unused while the block is allocated. wolfSSL_Free() drops that reference, so the hint goes back to the pool when its last block is freed.
  • wolfSSL_free() drops the SSL's reference through wc_HeapHint_Release().
  • Fixed IO buffers (WOLFMEM_IO_POOL_FIXED) belong to the SSL and hold no reference. IO pool buffers do hold one.
  • In realloc, the new block takes its reference before the old block drops its own, so a block that is the hint's last holder can still be reallocated. IO buffers are resized in place and keep their owner.
  • CTX, user and global hints are not reference counted.

Testing

  • The static heap session, ticket and peer cert chain tests now check the hint's ref count as the SSL, sessions and X509s are freed. A new case has an X509 as the last holder.
  • New test_wolfSSL_static_heap_io_refs (IO pool and fixed IO) and test_wolfSSL_static_heap_realloc_refs tests.
  • The tests check that curAlloc returns to its baseline (skipped with WOLFSSL_STATIC_MEMORY_LEAN).
  • Fixed test_wolfSSL_CTX_add_session_ext without WOLFSSL_TICKET_HAVE_ID. A TLS 1.2 ticket session has no session ID, so it can't be added to the cache, and the remove callback isn't called on resumption.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new regression tests need configuration fixes for builds without PEM parsing or TLS 1.2 client authentication.

2 open issues
What changed in this PR

This PR fixes session memory ownership so retained sessions do not depend on a connection’s freed static-memory heap hint.

Changes:

  • Create sessions using the context heap during initialization and reset.
  • Use the session heap for tickets, ticket nonces, and retained peer certificates.
  • Add regression tests for session retention, duplication, resumption, and cleanup.
File Description
tests/​api.c Adds static-memory session lifetime and cleanup tests.
src/​ssl.c Uses the context heap when wolfSSL_clear() replaces a session.
src/​ssl_api_cert.c Allocates peer-chain certificates from the session heap.
src/​internal.c Corrects session creation and ticket-related heap usage.

Review effort: Balanced

Comment thread tests/api.c Resolved
Comment thread tests/api.c Resolved

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #11899

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

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

@aidankeefe2022
aidankeefe2022 marked this pull request as ready for review October 26, 2026 20:51
@github-actions

Copy link
Copy Markdown

retest this please

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

@danielinux danielinux changed the title Fenrir 15782 (HIGH): Allocate session memory from the CTX heap, not the SSL heap Fenrir 15782: Allocate session memory from the CTX heap, not the SSL heap Oct 27, 2026

@julek-wolfssl julek-wolfssl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not a maintainable solution. Caller sites should not have knowledge about which hint to use. I would prefer to store a reference counter in WOLFSSL_HEAP_HINT and let the allocator figure out when it can be released. Attached is a detailed description of what I envision.

pr11899-heap-hint-refcount.md

Comment thread wolfssl/wolfcrypt/memory.h Resolved
Comment thread src/x509.c Resolved
@aidankeefe2022 aidankeefe2022 changed the title Fenrir 15782: Allocate session memory from the CTX heap, not the SSL heap Fenrir 15782: Static blocks are ref counted and freed by last ref Oct 28, 2026
@aidankeefe2022 aidankeefe2022 changed the title Fenrir 15782: Static blocks are ref counted and freed by last ref Fenrir 15782: wolfSSL Static blocks are ref counted and freed by last ref Oct 28, 2026
@aidankeefe2022

Copy link
Copy Markdown
Member Author

retest this please

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #11899

Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Coverage: 5 of 6 in-scope changed file(s) opened by the reviewer; not opened: tests/api.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

Comment thread tests/api.c Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The API tests still call XREALLOC when WOLFSSL_STATIC_MEMORY and WOLFSSL_NO_REALLOC are enabled, causing link failure because wolfSSL_Realloc() is compiled out.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread tests/api.c Resolved
@aidankeefe2022

Copy link
Copy Markdown
Member Author

added some guards to tests this just passed CI before that change assigning to wolfSSL_Bot

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Allocator lifetime changes require human review and concurrent runtime validation across atomic and mutex-backed reference counts, since the added regressions cover only sequential lifetimes.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Comment thread wolfssl/wolfcrypt/memory.h Outdated
wc_Memory* inBuf;
byte haFlag; /* flag used for checking handshake count */
#endif
wolfSSL_Ref refCount; /* SSL + each allocated block */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually since the memory_mutex is always held when touching the heap hint I think this can be a plain word32. This should make the code much simpler. Sorry for the confusion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reverted it!

With WOLFSSL_STATIC_MEMORY, each SSL gets its own heap hint, and
wolfSSL_free() releases it. Some objects allocated from that hint can
live longer than the SSL: a session kept with wolfSSL_get1_session(),
the server's peer cert stored in the session, or an X509 returned by
wolfSSL_get_peer_certificate(). After wolfSSL_free(), these objects
point at a hint that no longer exists.

Update the static heap session, ticket and peer cert chain tests to
check the hint's refCount as the SSL, sessions and X509s are freed.
Add a case where an X509 is the last holder. Check that curAlloc
returns to its baseline (except with WOLFSSL_STATIC_MEMORY_LEAN).

Fix test_wolfSSL_CTX_add_session_ext without WOLFSSL_TICKET_HAVE_ID:
a TLS 1.2 ticket session has no session ID, so it can't be added to
the cache, and the remove callback isn't called on resumption.
Static blocks now keep a ref count and only free when all holders are
done holding.
@aidankeefe2022
aidankeefe2022 force-pushed the ssl-sess-fenrir branch 2 times, most recently from fc8df7a to 76cc88d Compare October 29, 2026 19:16
@aidankeefe2022
aidankeefe2022 requested review from wolfSSL-Fenrir-bot and a balanced review from Copilot October 29, 2026 21:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The test guard enables calls to an excluded helper when WOLFSSL_NO_MALLOC is defined alongside WOLFSSL_STATIC_MEMORY, OPENSSL_EXTRA, and SESSION_CERTS, causing an undefined-function build failure.

1 open issue

Review effort: Balanced

Comment thread tests/api.c Resolved

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #11899

Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Coverage: 3 of 4 in-scope changed file(s) opened by the reviewer; not opened: tests/api.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

Comment thread tests/api.c Resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants