Repository navigation
Conversation
dispose_sig() and dispose_KEM() freed the native liboqs object but left its pointer in the Java object. Calling dispose twice freed it twice, and calling any other method afterwards used freed memory. Clear the handle when freeing, skip the free if it is already clear, and throw a RuntimeException from methods that need the native object once it has been disposed. Signed-off-by: Douglas Stebila <dstebila@uwaterloo.ca> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jimouris
left a comment
jimouris
left a comment
There was a problem hiding this comment.
LGTM!
Left some nits from an AI pass.
| if (sig == NULL) return; | ||
| OQS_SIG_free(sig); | ||
| // Clear the handle so that the freed object cannot be used again | ||
| setHandle(env, obj, NULL, "native_sig_handle_"); |
There was a problem hiding this comment.
😬 we had missed this, good catch.
| */ | ||
| private void check_not_disposed() throws RuntimeException { | ||
| if (native_sig_handle_ == 0) { | ||
| throw new RuntimeException("Signature has been disposed"); |
There was a problem hiding this comment.
(🤖 pass) Nit: IllegalStateException fits use-after-dispose better than a plain RuntimeException.
If the contract is that a disposed object is fully unusable, print_details(), print_signature(), and the export-key methods should call this too. They do not SIGSEGV after a normal dispose (alg_details_ is already cached; the secret is only wiped), but they still succeed while sign/verify throw.
| */ | ||
| private void check_not_disposed() throws RuntimeException { | ||
| if (native_kem_handle_ == 0) { | ||
| throw new RuntimeException("KeyEncapsulation has been disposed"); |
There was a problem hiding this comment.
Same as on Signature: consider IllegalStateException, and print_details() / export_* if you want dispose to apply to the whole object.
| assertDisposed(() -> signer.verify(message, signature, context, signer_public_key)); | ||
| } | ||
|
|
||
| private static void assertDisposed(Executable executable) { |
There was a problem hiding this comment.
If print/export should also fail after dispose, assert that here. Now they still succeed on cached/wiped Java state.
dispose_sig()anddispose_KEM()free the native liboqs object but leave its pointer in the Java object. Calling dispose twice frees the object twice, which crashes the process. Calling any other method after dispose uses freed memory. Both happen only through the calling application's own sequence of calls.Changes
RuntimeException("Signature has been disposed")orRuntimeException("KeyEncapsulation has been disposed")once the object has been disposed:Signature:generate_keypair, and both overloads ofsignandverify.KeyEncapsulation: both overloads ofgenerate_keypair, plusencap_secretanddecap_secret.This does not make it safe to dispose an object in one thread while another thread is using it.
Testing
There are 4 new tests in
SigTestandKEMTest. Before the fix, the double-dispose tests crashed the JVM, and the use-after-dispose tests found that no exception was thrown.Tested locally with
mvn packageon macOS (arm64, JDK 27) against liboqsmain(8c41c85), where all the new tests pass. The fix does not depend on how liboqs handles a NULL object.Until #36 is merged, three existing
testAllSigscases fail on both this branch andmaster, for ML-DSA-44-extmu, ML-DSA-65-extmu and ML-DSA-87-extmu. liboqs signs with these only when the message is the 64-byte μ, and the test signs a 27-byte message. Merged with #36,mvn packagepasses with no errors.This PR is independent of #37, and the two merge cleanly together.
🤖 Generated with Claude Code