fix: Free pre-existing allocation in s2n_alloc to prevent memory leak… - #5990
Open
alexw91 wants to merge 1 commit into
Open
fix: Free pre-existing allocation in s2n_alloc to prevent memory leak…#5990alexw91 wants to merge 1 commit into
alexw91 wants to merge 1 commit into
Conversation
alexw91
force-pushed
the
fix-alloc-memleak
branch
2 times, most recently
from
July 16, 2026 22:53
14fa2af to
f1c0dbc
Compare
alexw91
requested review from
boquan-fang and
jmayclin
and removed request for
boquan-fang
August 10, 2026 20:27
alexw91
force-pushed
the
fix-alloc-memleak
branch
2 times, most recently
from
August 10, 2026 22:50
d0b44a3 to
ec636c6
Compare
jmayclin
reviewed
Aug 11, 2026
| * s2n_alloc allocates a new buffer of the requested size. If the blob | ||
| * already holds an allocation, it is freed first. | ||
| * | ||
| * s2n_realloc is preferred when growing an existing blob, as it may |
Contributor
There was a problem hiding this comment.
🙋 Okay, maybe this is a stupid question, but why don't we just delete s2n_alloc and switch everything to use s2n_realloc?
Contributor
Author
There was a problem hiding this comment.
s2n_realloc requires the blob to already be growable (b->growable is explicitly set). s2n_realloc rejects non-growable blobs with S2N_ERR_RESIZE_STATIC_BLOB.
s2n_alloc zeroes the blob first (which makes it pass the growable check), so it works on blobs in any state, including ones previously set up with s2n_blob_init() pointing at stack memory.
… s2n_alloc() zeroed the blob struct (including b->allocated) before calling s2n_realloc(). Since s2n_realloc only frees the previous buffer when b->allocated is non-zero, the previous b->data pointer was silently orphaned on every call to s2n_alloc on an already- allocated blob. This was reachable via QUIC transport parameters during HRR, s2n_server_key_share_recv, s2n_client_psk_recv, and s2n_kem_recv_public_key. Fix: Check b->allocated before zeroing and call s2n_free() if a pre-existing allocation exists.
alexw91
force-pushed
the
fix-alloc-memleak
branch
from
August 14, 2026 21:44
ec636c6 to
312310a
Compare
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.
Goal
Free pre-existing allocations in
s2n_allocto prevent memory leaks when called on an already-allocated blob.Why
s2n_alloc()zeroes the blob struct (includingb->allocated) before callings2n_realloc(). Sinces2n_realloconly frees the previous buffer whenb->allocatedis non-zero, the previousb->datapointer is silently orphaned. This is reachable via QUIC transport parameters during HelloRetryRequest,s2n_server_key_share_recv,s2n_client_psk_recv, ands2n_kem_recv_public_key.How
Check
b->allocatedbefore zeroing and calls2n_free()if a pre-existing allocation exists. This is a 4-line fix inutils/s2n_mem.c. The header comment inutils/s2n_mem.hand the doxygen comment inutils/s2n_mem.care updated to reflect the new semantics (the old docs warned that callings2n_allocon an allocated blob would leak).Callouts
s2n_allocwas explicitly documented as leaking on re-use ("calling s2n_alloc on a blob that already has memory allocated will leak memory"). Callers that relied on this were already buggy, so this is safe.s2n_reallocremains the preferred API for growing an existing blob since it can reuse the buffer without a new allocation.Testing
Tests are added to the existing
s2n_mem_test.cusing counting malloc/free callbacks:s2n_allocon the same blob frees the first allocation (malloc=2, free=1 after second alloc)All new assertions fail without the fix and pass with it.
Related
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.