Skip to content

chore: delete unused s2n_stuffer_alloc_ro functions#5757

Merged
WesleyRosenblum merged 3 commits intoaws:mainfrom
firedog1234:fix-stuffer-mmap-unmap
Mar 4, 2026
Merged

chore: delete unused s2n_stuffer_alloc_ro functions#5757
WesleyRosenblum merged 3 commits intoaws:mainfrom
firedog1234:fix-stuffer-mmap-unmap

Conversation

@firedog1234
Copy link
Copy Markdown
Contributor

@firedog1234 firedog1234 commented Feb 23, 2026 •
edited by WesleyRosenblum
Loading

Goal

Remove the unused functions s2n_stuffer_alloc_ro and s2n_stuffer_alloc_ro_from_file

Why

s2n_stuffer_alloc_ro_from_fd uses mmap() to map a file into memory, but never calls munmap() to unmap it when the stuffer is freed. We could fix this, but since the code is unused it is better to delete it.

How

  • Delete s2n_stuffer_alloc_ro_from_fd() and s2n_stuffer_alloc_ro_from_file() functions
  • Remove unused #include <sys/mman.h> and related includes
  • Delete CBMC proofs for the removed functions

Callouts

  • This removes unused code that had a memory leak, rather than fixing it. Since the functions are never called, this is the cleanest solution.

Testing

  • Verified that the functions are not used anywhere in the codebase. All existing tests continue to pass.

Related

Fixes #2147

@firedog1234 firedog1234 changed the title Fix memory leak: unmap mmap'd memory in s2n_stuffer_free Fix: memory leak, unmap mmap'd memory in s2n_stuffer_free Feb 23, 2026
@boquan-fang
Copy link
Copy Markdown
Contributor

boquan-fang commented Feb 23, 2026

Hello @firedog1234,

Thanks for contributing to s2n-tls! I have assigned reviewers for this PR. To get started, please fix those errors detected by our CI. Thanks!

@jmayclin
Copy link
Copy Markdown
Contributor

jmayclin commented Feb 23, 2026

From a quick check over of the codebase, it looks s2n_stuffer_alloc_ro_from_fd isn't ever used? If that's the case, I think the best fix is to just remove it 🙂

jmayclin
jmayclin reviewed Feb 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

Already left this comment, but officially leaving a review so you can hit the re-request button.

From a quick check of the codebase, this code doesn't seem to be used.

s2n_stuffer_alloc_ro_from_fd is only ever called by s2n_stuffer_alloc_ro_from_file, and s2n_stuffer_alloc_ro_from_file isn't ever called.

In which case the best solution is to just delete the code 🙂

@firedog1234 firedog1234 force-pushed the fix-stuffer-mmap-unmap branch from afe3751 to b94766a Compare February 28, 2026 02:17
@firedog1234
Remove unused s2n_stuffer_alloc_ro_from_fd functions
9dede6e 
These functions use mmap() but never call munmap(), causing a memory
leak. Since they are not used anywhere in the codebase, the best
solution is to remove them entirely.

Fixes aws#2147
@firedog1234 firedog1234 force-pushed the fix-stuffer-mmap-unmap branch from b94766a to 9dede6e Compare February 28, 2026 02:28
@firedog1234 firedog1234 requested a review from jmayclin March 1, 2026 23:56
@firedog1234 firedog1234 changed the title Fix: memory leak, unmap mmap'd memory in s2n_stuffer_free fix: memory leak, unmap mmap'd memory in s2n_stuffer_free Mar 2, 2026
@WesleyRosenblum WesleyRosenblum changed the title fix: memory leak, unmap mmap'd memory in s2n_stuffer_free chore: delete unused s2n_stuffer_alloc_ro functions Mar 4, 2026
@maddeleine maddeleine removed their request for review March 4, 2026 21:12
@WesleyRosenblum WesleyRosenblum added this pull request to the merge queue Mar 4, 2026
Merged via the queue into aws:main with commit ce3ade6 Mar 4, 2026
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmayclin jmayclin jmayclin approved these changes

@WesleyRosenblum WesleyRosenblum WesleyRosenblum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mmaped data is never unmapped

5 participants

@firedog1234 @boquan-fang @jmayclin @WesleyRosenblum @CarolYeh910