Skip to content

BUG: Report XML read errors instead of treating them as end of input - #42

Merged
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-xml-read-errors
Sep 23, 2026
Merged

hjmjohnson merged 2 commits into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-xml-read-errors

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

axml_read_file()'s read loop:

blen = fread(buf, 1, bsize, fp);
...
done = blen < (unsigned)bsize;

A short read is the loop's only stopping condition, so an I/O failure is
indistinguishable from reaching the end of the file: the parse stops
early and the caller is handed whatever was parsed so far, with no
indication that the rest of the document was never read. ferror() is now
checked and the failure reported.

The loop also called fread() once more after a file ending exactly on a
buffer boundary; testing feof() as part of the stopping condition removes
that read, which the clang static analyzer flags as a read at EOF.


Interface impact: none. On the union of all these changes, configured with USE_FSL_CODE=ON and USE_CIFTI_CODE=ON: all 448 exported symbols across libniftiio, libnifti2, libznz, libfslio, libnifticdf and libcifti are identical to master under nm -D --defined-only, and all ten installed headers are identical under gcc -E -P. Under gcc -dM -E one macro definition differs, intentionally and only in text: #61 makes FSL_RADIOLOGICAL read (-1) so it is safe inside an expression. Its value is still -1, checked by compiling against each installed fslio.h and printing it.

Verification. This branch: builds with gcc 16.1.1, ctest unchanged from master (2 of 345 fail on master itself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8, ctest 345/345 under each, and the whole suite under valgrind memcheck with --trace-children=yes gives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.

Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9 -Wsign-conversion (5 in fslio.c for #22, 2 in nifti2_io.c and 2 in nifti_tester001.c for #24) and 8 -Wcalloc-transposed-args in nifti_findhdrname and nifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.

One of a set of independent, single-purpose PRs. Each bases on master and can be merged on its own, in any order.

The full set of PRs (35)

The union of all of them is on the fork as all-changes, if you want to build and test the lot at once.

CI and build

Configuration and documentation

Defects

Warning and check classes

This was referenced Aug 15, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Rebased onto master (b4876bf) and added red-green coverage for the read-error path. 364/364 tests pass.

Message and comment cleanup

Removed the AI Co-Authored-By: trailer and the Claude-Session: chat URL. The added source comment was cut from three narrating lines to one, per the project's comment rule. The functional diff is unchanged.

Tests and red proof

Two tests in cifti/CMakeLists.txt:

  • cifti_xml_read_error — the input is cifti/testdata, a directory. It opens, and the first fread() fails with EISDIR. Asserts failed to read XML file.
  • cifti_xml_read_ok — companion. A real document must still parse, and must not report a read error, so a change that reported an I/O failure unconditionally would not pass.

The assertion is on the diagnostic, not the exit status: the tool exits 1 both before and after the fix, so exit status proves nothing here.

With cifti/afni_xml.c reverted to master:

362: ** no element found at line 1
362: ** failed to read XML data from '.../cifti/testdata'
1/2 Test #362: cifti_xml_read_error .............***Failed  Required regular expression not found. Regex=[failed to read XML file
2/2 Test #363: cifti_xml_read_ok ................   Passed

The wrong diagnostic is exactly the symptom — expat was handed a zero-length final buffer and blamed the document for the I/O error. With the fix:

** failed to read XML file '<path>'
** failed to read XML data from '<path>'
Not covered

The feof() half of the change removes one fread() that can only return zero, on a file whose length is an exact multiple of the parse buffer. That has no observable effect — it is a static-analyzer finding only — and has no test. Only the ferror() half is covered.

gdevenyi and others added 2 commits September 23, 2026 06:47
axml_read_file()'s read loop:

    blen = fread(buf, 1, bsize, fp);
    ...
    done = blen < (unsigned)bsize;

A short read is the loop's only stopping condition, so an I/O failure is
indistinguishable from reaching the end of the file: the parse stops
early and the caller is handed whatever was parsed so far, with no
indication that the rest of the document was never read.  ferror() is now
checked and the failure reported.

The loop also called fread() once more after a file ending exactly on a
buffer boundary; testing feof() as part of the stopping condition removes
that read, which the clang static analyzer flags as a read at EOF.
A directory opens but cannot be read, so afni_xml_tool on cifti/testdata
reaches the ferror() branch; the test pins the diagnostic rather than
the exit status, which was already non-zero before the fix.  A second
test parses a real document and forbids that diagnostic, so a change
that reported a read error unconditionally would not pass.
@hjmjohnson
hjmjohnson force-pushed the pr/fix-xml-read-errors branch from bbf051b to 9939264 Compare September 23, 2026 11:47
@hjmjohnson
hjmjohnson merged commit 283c5d4 into InsightSoftwareConsortium:master Sep 23, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants