Reject a matrix file whose modulus does not fit in Scalar - #83
Merged
Merged
Conversation
SparseMatrix::read reads the modulus with readOne<uint32> and returns it as Scalar, which is uint16. A file claiming 65637 was therefore read as 101, and nothing downstream could tell: the primality check PR #74 added sees only the truncated value, 101 is prime, so the reduction ran over a field the file never named and the result was written back out claiming 101. Demonstrated with two hand-built .brmat files, identical but for the modulus field, 65637 and 101: $ mgb matrix big.brmat $ mgb matrix small.brmat $ cmp big.rbrmat small.rbrmat && echo identical identical Check the value where the uint32 is still intact, through the same error path the rest of read already uses, so mgb matrix reports it and exits rather than producing a wrong answer: ERROR: The matrix file has modulus 65637, which does not fit in the 16 bits that this file format stores coefficients in. Narrow in practice, as the file format cannot represent such a field anyway -- entries are uint16, and write() takes a Scalar so nothing mgb produces can trigger it. The point is that a file that says something we cannot honour is now refused instead of quietly reinterpreted. Verified: the new test fails without the check and passes with it, both builds pass 248/248, the CLI reproduction above now errors out on the 65637 file and still succeeds on the 101 one, and autotools make distcheck passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
modulus comes from readOne<uint32> and the return type is Scalar, which is uint16, so returning it narrows implicitly. That is safe now that the previous commit rejects a value too large to fit, and the cast says so rather than leaving a reader to check. No functional change: the conversion happened already. It silences a -Wconversion warning, which this project does not enable today, but doc/description.txt asks for a build target with all warnings on and treated as errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-torrance
added a commit
that referenced
this pull request
Sep 18, 2026
PR #74 put the primality check in MatrixAction.cpp, which is the one place it cannot be tested: mathicgb-unit-tests links no src/cli object and there is no CLI-level harness, so the check shipped uncovered and could not be given a test where it stood. SparseMatrix::read is where the modulus comes off the file, and it already rejects the file for the other thing that can be wrong with that field -- a value too large for Scalar, from PR #83 -- through the same error path. The primality check belongs beside it. QuadMatrix::read needs nothing of its own. It reads its four submatrices through SparseMatrix::read, so all four are checked, and the section of the review that asked for this assumed two changes where one does. Moving it also covers a read the CLI never checked at all: the reference .rbrmat that a second run compares its output against. What the CLI keeps is the file name, which read() cannot know, having only a FILE*. readMatrixFile opens the file, reads it, and on an error says which file it was before letting the exception past: While reading composite.brmat: ERROR: The modulus 100 is not prime. MathicGB only supports prime fields. Rethrowing rather than composing a new message matters: mathic's reportError has already put "ERROR: " into what(), so folding that text into a second reportError prints the word twice. This way PR #83's oversized-modulus error gets the file name too, for free. The three call sites each lose their CFile boilerplate to the same helper, and mgb matrix takes a list of files, so naming the file is worth keeping. Two tests, one per entry point, both of which fail without the check. A composite modulus needs no patching to get into a file, unlike an oversized one -- write() does not check either. Verified: 253/253 in cmake Release and Debug, no build warnings, and mgb matrix refuses a composite-modulus .brmat with exit 255 while a prime one reduces and writes its output as before. Co-Authored-By: Claude Opus 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.
SparseMatrix::readreads the modulus withreadOne<uint32>and returns it asSparseMatrix::Scalar, which isuint16. A file claiming 65637 was read as101.
Nothing downstream could tell. The primality check PR #74 added sees only the
truncated value, and 101 is prime, so the reduction ran over a field the file
never named — and the result was written back out claiming 101.
It is silent corruption, not a misread
Two hand-built
.brmatfiles, identical but for the modulus field, 65637 and101:
Reading the header of
big.rbrmatback shows modulus 101. The 65637 is gonewith no diagnostic anywhere.
The fix
Check the value where the
uint32is still intact, through the same error paththe rest of
readalready uses, somgb matrixreports it and exits instead ofproducing a wrong answer:
This is narrow in practice, and worth saying so:
writetakes aScalar, sonothing mgb produces can trigger it, and entries are
uint16, so a modulusabove 2^16 is not representable in this format regardless. The point is that a
file asserting something we cannot honour is now refused rather than quietly
reinterpreted.
Two commits
51463eb— the check, and a regression test. The test patches themodulus field of a written file directly, since
writetakes aScalarandcannot produce an out-of-range one. It fails without the check.
eedaf0f—return static_cast<Scalar>(modulus);. No functional change:the narrowing already happened implicitly, and it is safe now that the
previous commit rejects anything too large. It silences a
-Wconversionwarning, which this project does not enable today, though
doc/description.txtasks for a build target that would.Each builds and passes standalone.
Verification
101 one.
make distcheckpasses.Not included
The review that found this also noted that PR #74's primality check lives in
src/cli/MatrixAction.cpp, where it cannot be unit tested at all —mathicgb-unit-testslinks nosrc/cliobject. Moving that validation intoSparseMatrix::readandQuadMatrix::readwould fix that, but it moves apolicy decision from the CLI into the library and deserves its own PR.
🤖 Generated with Claude Code