Reject a composite modulus before a Pimpl exists, fixing a leak and a double free - #90
Merged
Merged
Conversation
GroebnerConfiguration and mgbi::StreamStateChecker each allocated their Pimpl in the mem-initializer list and then checked the modulus in the constructor body. A constructor that throws never reaches its destructor, so the Pimpl was already built and nothing owned it: - GroebnerConfiguration leaked it. RejectsCompositeModulus drives that path five times, so the test suite has leaked on every run since it was added. - StreamStateChecker caught its own check, deleted the Pimpl and did not rethrow, so the constructor completed with a dangling pointer that the destructor freed a second time. Only direct construction reaches this, since a GroebnerConfiguration has already refused a composite modulus by then; in Release it has been a double free all along. Both checks now run in their Pimpl's own constructor. A Pimpl whose constructor throws is never completed and never destroyed, and the new-expression frees its storage, so neither outer constructor has a cleanup path to get wrong. StreamStateChecker's try/catch goes, and a composite modulus passed to it directly now throws, as the check always said it should. The new StreamCheckerRejectsCompositeModulus test covers that, and on master trips ASan's double-free report. The two checks were written differently: GroebnerConfiguration built "Modulus N is not prime. MathicGB only supports prime fields." and reported it with mathic::reportError, while StreamStateChecker threw invalid_argument as a stream protocol error without saying which modulus. Both now call checkModulusIsPrime, a new function beside isPrime in PrimeField.hpp, so they report the same message the same way. A composite modulus is bad input rather than a protocol violation, so the checker's error is now a MathicException, the runtime_error subclass GroebnerConfiguration already threw; its actual protocol errors are still invalid_argument. For GroebnerConfiguration the move is also what keeps Debug working once the Pimpl is freed: its destructor asserts debugAssertValid(), which requires a nonzero modulus, and the leak was the only reason a Pimpl holding a rejected modulus had never been destroyed. The Pimpl pointers stay raw rather than becoming unique_ptr. mathicgb.h keeps its members free of standard library types so that a caller and the library built against different STLs still agree on the layout, and with the checks moved, unique_ptr would fix nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MathicIO::readBaseField and SparseMatrix::read each built the "is not prime" message by hand, the last two copies of it now that the library interface uses checkModulusIsPrime. They had drifted: the matrix reader said "The modulus N", every other site "Modulus N". readBaseField's own charac < 2 guard goes too. Its coefficient type is long and the scanner accepts a sign, so a negative modulus reaches it, and checkModulusIsPrime tests modulus < 2 in the caller's type before converting to uint64 for exactly that reason. -7 is still reported as "Modulus -7 is not prime". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Both library constructors that validate a modulus allocated their
Pimplfirst and checked second. A constructor that throws never runs its
destructor, so each one had to clean up by hand, and they got it wrong in
opposite directions.
1.
GroebnerConfigurationleaked on a composite modulusIt allocated the
Pimpl, then rejected the modulus withmathic::reportError, and nothing freed it. Under ASan on master,RejectsCompositeModulusalone reportsonce per rejected modulus, five times, on every run of the suite.
2.
StreamStateCheckerdouble freed on a composite modulusNo rethrow, so the constructor completed with
mPimpldangling and thedestructor freed it again. Every other
catch (...)in the sources cleans upand rethrows, so this was an omission. It is reachable only by constructing
mgbi::StreamStateCheckerdirectly -- through the public interface themodulus comes from a
GroebnerConfiguration, which has already refused it --but
IdealStreamChecker<Stream>takes its modulus from any stream, so it isnot purely hypothetical either.
The fix: check before the
PimplexistsBoth checks move into their
Pimpl's own constructor. APimplwhoseconstructor throws is never completed and never destroyed, and the
new-expression frees its storage, so neither outer constructor has a cleanup
path left to get wrong. The try/catch goes.
For
GroebnerConfigurationthis is also what keeps Debug working.Pimpl::~PimplassertsdebugAssertValid(), which requires a nonzeromodulus; the leak was the only reason a
Pimplholding a rejected modulus hadnever been destroyed. Freeing it after the check would trip that assert in
RejectsCompositeModulus.The
Pimplpointers stay raw.std::unique_ptrwas the first attempt, andwas dropped:
mathicgb.hkeeps its members free of standard library types sothat a caller and the library built against different STLs agree on the
layout, and once the checks move,
unique_ptrfixes nothing. The publicheader is unchanged.
One message for a composite modulus
The two checks also reported differently --
GroebnerConfigurationsaid"Modulus N is not prime. MathicGB only supports prime fields." as a
MathicException, the checker said "The modulus must be prime" as aninvalid_argumentwithout saying which. The same message was built by hand intwo more places,
MathicIO::readBaseFieldandSparseMatrix::read, and thematrix reader's had drifted to "The modulus N".
checkModulusIsPrime, besideisPrimeinPrimeField.hpp, now owns it, andall four sites call it. It is a template so that
readBaseField, whosecoefficient type is
longand whose scanner accepts a sign, still rejects-7as "Modulus -7 is not prime" rather than converting it to a hugeunsigned value first.
User-visible changes:
MathicException(
runtime_error), likeGroebnerConfiguration's. A composite modulus isbad input rather than a protocol violation; the checker's actual protocol
errors are still
invalid_argument.rather than "The modulus N is not prime".
Commits
583a683-- the two constructors, the new function, and a regression test,StreamCheckerRejectsCompositeModulus.04146b6--MathicIOandSparseMatrixswitch to the function.Verification
254/254 in cmake Release and Debug, GCC 13.3, no build warnings.
ASan, master against this branch:
RejectsCompositeModulusStreamCheckerRejectsCompositeModulusStreamStateChecker(4, 2, 1), standalone programmgb gbon an ideal file with modulus-7, and with4, reportsERROR: Modulus -7 is not prime. MathicGB only supports prime fields.andthe same for 4.
No autotools
distcheckrun locally; neither commit touches a build system andCI covers it.
🤖 Generated with Claude Code