Repository navigation
Make dispose idempotent and reject use after dispose - #38
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.
There was a problem hiding this comment.
Switched to IllegalStateException. export_public_key, export_secret_key, print_signature and print_details now throw after dispose too, so a disposed Signature can't be used at all. export_secret_key was the one that mattered most, since it was quietly returning the zeroed key.
The only public method without the check is create_sig_new, because it's public native. Calling it after dispose creates a new native object, so it brings the Signature back to life rather than touching freed memory.
| */ | ||
| 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.
There was a problem hiding this comment.
Same changes here, plus get_keypair_seed_length, which also only reads cached state. create_KEM_new is the same exception as create_sig_new.
| 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.
There was a problem hiding this comment.
Done. The use-after-dispose tests in SigTest and KEMTest now check that each of these methods throws IllegalStateException after dispose.
Use IllegalStateException rather than RuntimeException for use after dispose. Also check in the methods that don't use the native object (export_public_key, export_secret_key, the print methods and get_keypair_seed_length), so that a disposed object can't be used at all. Before this, export_secret_key returned the zeroed secret key. Signed-off-by: Douglas Stebila <dstebila@uwaterloo.ca> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @jimouris! I've pushed some revisions addressing your comments. If they look good to you, we can then merge this. |
Signed-off-by: Douglas Stebila <dstebila@uwaterloo.ca> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
LGTM! Feel free to merge it - I didn't in case you're still making edits as I saw you pushed one more commit after your comment. |
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
The native free functions clear the handle after freeing and do nothing if it is already clear, so a second dispose is harmless.
These methods throw
IllegalStateException("Signature has been disposed")orIllegalStateException("KeyEncapsulation has been disposed")once the object has been disposed:Signature:generate_keypair, both overloads ofsignandverify,export_public_key,export_secret_key,print_signatureandprint_details.KeyEncapsulation: both overloads ofgenerate_keypair,encap_secret,decap_secret,get_keypair_seed_length,export_public_key,export_secret_key,print_KeyEncapsulationandprint_details.The export, print and seed-length methods don't use the native object, but they are included so that a disposed object can't be used at all. Otherwise
export_secret_keywould return the zeroed key. Code that called any of them after dispose will now get an exception.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