fix(mem): Restore mlock allocator defaults on init and cleanup#5848
Open
alexw91 wants to merge 2 commits into
Open
fix(mem): Restore mlock allocator defaults on init and cleanup#5848alexw91 wants to merge 2 commits into
alexw91 wants to merge 2 commits into
Conversation
The original s2n_mem_cleanup (2016, 7486f47) reset use_mlock to its default enabled state so that a subsequent s2n_init would start with the hardened allocator. The 2020 callback refactor (0de8c41) replaced the use_mlock flag with function pointer callbacks but changed cleanup to unconditionally select no_mlock, while init only conditionally overwrote the callbacks when S2N_DONT_MLOCK was set or in unit tests. This meant a s2n_cleanup_final/s2n_init cycle in a normal process permanently dropped mlock and MADV_DONTDUMP hardening for all subsequent key and secret allocations. Extract the callback selection logic into s2n_mem_set_default_callbacks and call it from both init and cleanup, restoring the original reset-to-defaults behavior. Add a regression test that installs sentinel callbacks, simulates a non-unit-test reinit cycle, and verifies the callbacks are reset.
4bc193f to
6e585fc
Compare
jmayclin
approved these changes
Apr 24, 2026
maddeleine
reviewed
May 7, 2026
| EXPECT_OK(s2n_mem_get_callbacks(&ignored_init_cb, &ignored_cleanup_cb, | ||
| &observed_malloc_cb, &observed_free_cb)); | ||
| EXPECT_EQUAL(observed_malloc_cb, s2n_mem_sentinel_malloc_cb); | ||
| EXPECT_EQUAL(observed_free_cb, s2n_mem_sentinel_free_cb); |
Contributor
There was a problem hiding this comment.
Not sure if this is intentional, but you're not checking cleanup_cb and init_cb here.
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
Restore the mlock/MADV_DONTDUMP allocator defaults on both init and cleanup, so that a
s2n_cleanup_final/s2n_initcycle does not permanently drop memory hardening.Why
The original
s2n_mem_cleanup(2016,7486f4785) resetuse_mlockback to its default enabled state, so a subsequents2n_initwould start with the hardened allocator. The 2020 callback refactor (0de8c411e) replaced theuse_mlockflag with function pointer callbacks but changedcleanup_implto unconditionally select theno_mlockvariants, whileinit_implonly conditionally overwrote the callbacks whenS2N_DONT_MLOCKwas set or in unit tests. After as2n_cleanup_final/s2n_initcycle in a normal process, theno_mlockcallbacks set by cleanup persisted through init, silently droppingmlockandMADV_DONTDUMPhardening for all subsequent key and secret allocations.How
Extract the callback selection logic into a single
s2n_mem_set_default_callbackshelper that evaluatesS2N_DONT_MLOCKands2n_in_unit_test(), then selects the appropriate mlock or no-mlock callbacks. Boths2n_mem_init_implands2n_mem_cleanup_implnow call this helper, restoring the original reset-to-defaults behavior and eliminating the asymmetry introduced by the refactor.Callouts
cleanup_implunconditionally selectingno_mlockdoes not appear to have been an intentional design choice. The original 2016 commit message explicitly states "This would be the least surprising behavior for the caller" about resetting to defaults. This PR restores that intent.mlock_impl, which is correct for the very firsts2n_initcall (before any cleanup has run).s2n_mem_set_default_callbacksisstatic voidsince it cannot fail.Testing
Added a regression test in
s2n_init_test.cthat:s2n_in_unit_test(false)and unsetsS2N_DONT_MLOCKto simulate a production environment.s2n_mem_cleanup/s2n_mem_initdirectly (avoids triggering real mlock-backed allocations that could fail under restrictiveRLIMIT_MEMLOCKin CI).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.