ENH: Add regression tests for the bug fixes of the past two days - #80
Merged
Merged
Conversation
…atype The REJECT_COMPLEX block reads nim->datatype, and the test for nim being NULL sits immediately below it, so a header the converter refuses crashes the process in builds that define REJECT_COMPLEX. The refusal itself is correct and already reported; only the order is wrong. Found by the malformed-header tests added in this branch: the guards make the converter return NULL for exactly the inputs they cover, which is the case this block never handled.
A dim[0] outside 1..7 is used to index dim[] in nifti_convert_n2hdr2nim, so a header carrying a large negative value reads far outside the struct. The fixture is a 604-byte NIFTI-2 header holding such a value; without the check the tool segfaults in an ordinary Release build, so this test guards on every CI job rather than only the sanitizer one.
nifti_read_n1_hdr reads 348 bytes, so swapping the buffer as whatever width its magic names reaches past the end when the magic says n+2. The fixture is a big-endian NIFTI-1 header wearing an n+2 magic, which the sanitizer legs report as a stack-buffer-overflow without the fix. -check_hdr is the seam that reaches it; -disp_hdr does not.
act_mod_hdrs() and act_swap_hdrs() hold a 348-byte nifti_1_header and swapped it at the width its magic named, reading past the end for an n+2 magic. Both reuse the fixture added for the library-side check, and both rewrite their input, so each test runs against its own copy.
Three fixtures, because the two guards are reached separately: a product that wraps nvox, the NIFTI-2 form of the same, and one whose nvox fits while nvox * nbyper does not. Without the guards the tool accepts all three and reports a negative nvox. The regex pins which refusal fired; an exit-code test would not tell this rejection apart from any other error.
nifti_convert_nhdr2nim carries its own copy of the guards, so it needs its own tests; reverting the NIFTI-1 half alone leaves the nifti2 tests green and turns exactly these two red.
nifti_image_from_ascii overwrote fname and iname without releasing what they already held, so an ASCII header naming either attribute twice leaked the earlier copy. One fixture drives both libraries. The regex asserts the last value survives, so a fix that ignored the repeat instead of freeing the previous value could not pass.
loc_strnlen read one byte past the buffer it was given. The reachable caller is axml_read_buf, where the buffer is an extension's edata, allocated esize-8 and filled exactly; axml_read_file allocates one spare byte and absorbs the overread, so it is not the seam. The fixture is a NIFTI-1 image carrying a CIFTI extension whose payload is space-padded to hold no NUL. The overread is reported by the sanitizer legs; the build stays green without them.
The tool writes both members of the pair itself, so no data is needed. The driver asserts exit 1 and the diagnostic, because a library reports the clash to its caller rather than ending the process, and a plain add_test cannot tell that exit from the abort it replaced. It then removes the .gz and repeats, so an unconditional NULL return could not pass. Guarded on FSLSTYLE_NAME_CONFLICTS, which is off by default, so the test is registered only where the code is compiled.
hjmjohnson
force-pushed
the
test/regression-coverage
branch
from
September 22, 2026 12:19
fa0d6bf to
902db64
Compare
Two paths returned without releasing the header, and one without releasing the duplicated name: a write into a directory that refuses it, and a duplication that fails because the input is the other NIfTI version. Both are reachable from the shipped tools with no data beyond what -make_im writes. The script asserts only that the paths are reached and reported; the leak is what the memcheck and sanitizer legs see. It skips itself for root, whom a read-only directory does not stop.
c22_copy_image compares by content because gzip output is not reproducible across zlib implementations, but that property was only exercised where the system zlib is zlib-ng. Every CI leg ships stock zlib, so the comparison could revert to comparing compressed bytes and the suite would stay green. Extracting the helper lets it be tested directly. gzip -1 against -9 manufactures the byte difference on any zlib, and the two negative assertions pin that differing content is still reported as different and that the uncompressed path still works.
hjmjohnson
force-pushed
the
test/regression-coverage
branch
from
September 22, 2026 12:27
902db64 to
2fe4b60
Compare
hjmjohnson
marked this pull request as ready for review
September 22, 2026 12:33
Member
Author
|
The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: This PR's commits on the rewritten
The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves. |
This was referenced Sep 24, 2026
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.
Adds a regression test for each of the ten bug fixes that landed on
masterover the past two days, plus two fixes for bugs the tests themselves uncovered. Every test was verified by reverting its fix and watching it go red; none of these fixes has any coverage today, and the suite stays green with any of them reverted.What each test covers, and the observed red
7de7c95dim[0] rangenifti_tool_disp_nim_bad_dim0352297blibrary swap widthcheck_hdr_magic_n2+ the 2 tool testsfb83d99tool swap widthmod_hdr_magic_n2,swap_as_old_magic_n29ab39c6nvox overflownifti_toolrefusal tests10068b9nvox overflow, NIFTI-1nifti1_toolrefusal tests8de2af0ASCII repeated attributeascii_dup_attrtests744e751header failure pathsmod_hdr_errpaths0cf25aaloc_strnlen boundcifti_tool_unterminated_cextfb6035bambiguous filenameambiguous_hdrnametests510abc1content comparisonnii_cmp_selftestReverting
352297balso reddens the twofb83d99tests, because those tool paths read their header through the library. That is expected rather than an over-broad test.Two bugs the tests found
The
REJECT_COMPLEXblock readsnim->datatypebefore the NULL check below it. Six of these tests segfaulted underFSLSTYLE=ONwith every fix in place:Present in both libraries. The guards added over the past two days make the converter return NULL for exactly the malformed headers these fixtures carry, so every
REJECT_COMPLEXbuild crashed on a header it had just correctly refused. Only the order was wrong; the refusal itself was already right.cifti_toolnever frees the tree it parses.process()returned without releasing theafni_xml_tor thenifti_image. These are the first tests to runcifti_toolunder a sanitizer, and LeakSanitizer is on by default under AddressSanitizer on Linux but not on Apple, so it fails there only. The same fix is in #40 — drop that commit from this branch if #40 merges first.Fixtures and seams
Nine fixtures, 352 to 704 bytes, all in tree; none needs the external testing data. Each per-commit report records the Python that generates it, so the bytes are reproducible rather than opaque blobs. Two are shared: one header drives three tests across
352297bandfb83d99, and one drives both halves of the9ab39c6/10068b9pair.Seam precision worth knowing:
-check_hdrreaches the library swap;-disp_hdrdoes not.loc_strnlen,-as_cextis the wrong seam —axml_read_fileallocates one spare byte and absorbs the overread. Onlyaxml_read_buf, where the buffer is an extension'sedataallocatedesize-8and filled exactly, reaches it.cmake -Pdriver, because the expected status is exit 1 and plainadd_testcannot tell that from the abort it replaced. They are guarded onFSLSTYLE_NAME_CONFLICTS, so they register only where the code is compiled.mod_hdr_errpathscannot assert merely "the command failed": both invocations are expected to fail, and a sanitizer abort exits nonzero too. It asserts the expected diagnostic and the absence of a sanitizer report.Where a fix makes something stop happening, a companion assertion pins what must still happen — the recognized element still displays, the last repeated attribute still wins, an unambiguous name still resolves, differing content is still reported as different. Without those, an over-broad fix that refused everything would pass.
Verification
FSLSTYLE=ONEvery revert above was run in an isolated worktree with its own build tree, and the tree restored to 360/360 after each.
Two tests are only red under a sanitizer (
0cf25aa, and the two leak fixes), and two only underFSLSTYLE=ON. Those legs exist in CI, so the tests do run — but they are not guarded by the ordinary Release matrix, and that is stated rather than implied.