Fix StatefulSignature.free leaking the native OQS_SIG_STFL struct - #160
Open
stanleys12 wants to merge 1 commit into
Open
stanleys12 wants to merge 1 commit into
stanleys12 wants to merge 1 commit into
Conversation
free() detached the store callback and freed the secret key when the instance owns it, but it never called OQS_SIG_STFL_free, so the struct allocated by OQS_SIG_STFL_new in __init__ leaked for every instance, including the ones released through the context manager. KeyEncapsulation.free and Signature.free already call their OQS_KEM_free/OQS_SIG_free counterparts, and the class docstring lists free as the wrapper for OQS_SIG_STFL_free. Call OQS_SIG_STFL_free and clear the pointer, so a second free() is a no-op instead of a double free, and add a test that records the calls made to OQS_SIG_STFL_free. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Stanley Shen <sshen37@ucsc.edu>
This branch has not been deployed
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
1 participant
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.
StatefulSignature.free() detaches the store callback and frees the secret key when the instance owns it, but it never calls OQS_SIG_STFL_free, so the struct that OQS_SIG_STFL_new allocates in init leaks for every LMS/XMSS/XMSSMT instance, including the ones released through the context manager. The class docstring already lists
free | OQS_SIG_STFL_free, and the sibling KeyEncapsulation.free() and Signature.free() call OQS_KEM_free and OQS_SIG_free.free() now calls OQS_SIG_STFL_free and clears self._sig, so calling it twice (for example inside a
withblock and again afterwards) is a no-op instead of a double free.One leak in the same function is left untouched: the OQS_SIG_STFL_SECRET_KEY_free call is guarded by self._owns_secret, which init sets to False and nothing ever sets to True, so the secret key allocated by generate_keypair() or by passing secret_key= is still leaked. Fixing that means deciding who owns the secret key handle across generate_keypair(), _load_secret_key() and the store callback, which I did not want to fold into this change. I am happy to follow up with it if you tell me which ownership you want.
Added tests/test_stfl_sig.py::test_free, which wraps the library's OQS_SIG_STFL_free to record the pointers it receives and asserts a single call for free() followed by a second free(). It fails on main and passes with this change. The full tests.test_stfl_sig module (152 tests) passes locally against liboqs built with -DOQS_HAZARDOUS_EXPERIMENTAL_ENABLE_SIG_STFL_KEY_SIG_GEN=ON.
AI assistance (Claude) was used to draft this change;
nose2 tests.test_stfl_sigwas run locally with and without the fix.