BUG: Keep the XML skip depth at the element that started the skip - #65
Conversation
|
Pushed a second commit, plus an ASan demonstration of the first one. On master, 25 levels of nested XML against an That is the The second commit guards |
ebd77b6 to
9a32271
Compare
9a32271 to
deacbd7
Compare
deacbd7 to
737d736
Compare
|
Rebased onto current Rebase and commit-message cleanupThe ~29 shared-base commits are already on Message cleanup: removed the Test added, and the red proofNew fixture Red, with only the The same revert in a plain Release build fails too — Green with the fix, in both builds. Per the "a fix that stops something happening needs a companion assertion", the test pins |
afni_xml's parser holds sixteen levels of open elements in a fixed array and refuses to push a seventeenth by entering a skip block: epush() records the depth in xd->dskip and stops touching the stack until the matching pop. Two details of that bookkeeping are wrong. epush() overwrote dskip on every push past the limit, so the skip was recorded as starting at the innermost depth. epop() then cleared dskip before the stack was handled, so the pop that ends the skip fell through to "xd->stack[xd->depth-1] = NULL" and wrote a pointer eight bytes past the sixteen-entry array, which lives in the file-scope afni_xml_control. The next pop found dskip clear and dereferenced xd->stack[16]. epush() now keeps the outermost skip depth, since that is the depth whose pop ends the skip, and epop() clears dskip only after the element has been skipped. Depths within the limit are unaffected. The XML comes from the CIFTI extension of a NIfTI file, or from any file read by axml_read_file(), so the nesting is chosen by the input. Found by fuzzing axml_read_buf() with libFuzzer under AddressSanitizer.
process_popped_element() read xd->stack[xd->depth-1] and dereferenced it without a test. A slot is filled by the matching epush(), and an element that was skipped never fills one, so the read is only safe because the preceding commit keeps the skip running to the element that started it. Guard it rather than rely on that: check the depth is inside the stack, and that the slot holds a struct with a name, before the strcmp().
deep_nesting.xml nests twenty elements inside a CIFTI extension whose stack holds sixteen, which is the shape that walked off the end of xd->stack. The test drives it through cifti_tool and pins that the BrainModel sibling is still summarized, so a fix that refused the whole document would not pass.
737d736 to
c81291f
Compare
|
Rebased onto current One thing a reviewer needs to know up front: the two fix commits are a matched pair and neither is sufficient alone. Taking one without the other produces a tree that passes the new test while remaining broken. Red proof, and why both commits are requiredI reverted each hunk independently before reverting both, in a plain Release build with no sanitizer:
That is worth stating because "each commit stands alone" is normally the property you want, and here it does not hold: the guard stops a nested skip from resetting Under AddressSanitizer with both reverted, the diagnostic is precise: The fixture, and its companion assertion
It also carries a legal-depth The input is the XML's own nesting depth, so this is reachable from a malformed or hostile CIFTI file rather than only from a programming error. Verification
Rebased cleanly onto current CI on this push is still running at the time of writing; the local results above are what I can state now. Both Windows legs are among the queued checks, which this PR has not previously been built against. One correction to the commit textThe original commit inlined a stack trace whose line numbers had drifted from the code — it cited |
0e0033d
into
InsightSoftwareConsortium:master
2 participants
afni_xml's parser holds sixteen levels of open elements in a fixed
array, and refuses to push a seventeenth by entering a "skip block":
epush() records the depth in xd->dskip and stops touching the stack
until the matching pop. Two details of that bookkeeping are wrong, and
together they walk off the end of the array.
epush() overwrites dskip on every push past the limit, so after
elements at depth 17 and 18 the skip is recorded as starting at 18.
epop() then clears dskip before the stack is handled, so the pop that
ends the skip -- depth 18 -- falls through to
and writes a pointer eight bytes past the sixteen-entry array, which
lives in the file-scope afni_xml_control. The pop of depth 17 that
follows finds dskip clear and calls process_popped_element(), which
reads xd->stack[16] and dereferences it.
Eighteen nested elements are enough. On '' repeated eighteen times
and closed, AddressSanitizer reports:
The XML comes from the CIFTI extension of a NIfTI file, or from any
file read by axml_read_file(), so the nesting is chosen by the input.
Two changes: epush() keeps the outermost skip depth rather than the
innermost, since that is the depth whose pop ends the skip; and epop()
clears dskip only after the element has been skipped, which puts the
stack assignment in the branch that owns the stack. Depths within the
limit follow the same path as before.
Found by fuzzing axml_read_buf() with clang's libFuzzer under
AddressSanitizer.
How to reproduce, and the sanitizer builds used, are in the commit message. Found with clang libFuzzer harnesses over
nifti_image_read(), the two header converters,nifti_image_from_ascii()andaxml_read_buf(), run under AddressSanitizer, UndefinedBehaviorSanitizer and MemorySanitizer.One commit on master, independent of my other open PRs.
ctestpasses on master with this branch alone (343/343, excluding thenifti_c22_copy_imagefailure that #31 fixes), and on the union of all my branches under ASan+UBSan, TSan and MSan.