csv-core: fix NFA read_record resumption after OutputFull - #430
Open
maxtaran2010 wants to merge 1 commit into
Open
csv-core: fix NFA read_record resumption after OutputFull#430maxtaran2010 wants to merge 1 commit into
maxtaran2010 wants to merge 1 commit into
Conversation
When read_record_nfa was resumed after returning OutputFull (i.e. called a second time for the same record because the output buffer was full), it initialised the local nout counter to self.output_pos instead of 0. This caused two distinct bugs: 1. Field bytes were written at the wrong offset in the output slice. The caller passes &mut output[outlen..], so the first writable byte is always index 0. Starting nout at output_pos made the NFA write at output[output_pos], which is output_pos bytes too far into the buffer and can cause an out-of-bounds write or data corruption. 2. The returned nout was inflated by output_pos. The caller adds the returned nout to its own outlen accumulator, so the next call would pass a slice starting outlen too far along, corrupting all subsequent field data. The DFA implementation (read_record_dfa) handles this correctly: it starts nout at 0, computes field-end positions as self.output_pos + nout, and updates output_pos as self.output_pos + nout. This commit makes the NFA follow the same pattern. A regression test is added that forces two read_record calls for a single record by supplying a 3-byte output buffer for a 5-byte first field, and then verifies that both the written bytes and the field-end positions are correct after the resume. Co-Authored-By: Claude Sonnet 4.6 <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.
What this fixes
read_record_nfainitialised its localnoutcounter toself.output_posinstead of0. When the function is called a secondtime for the same record (because the output buffer was too small and
OutputFullwas returned), this caused two bugs:Bug 1 — bytes written at the wrong offset.
The caller always passes
&mut output[outlen..], so byte index 0 of thereceived slice is the right place to start writing. Starting
noutatoutput_posmade the NFA write atoutput[output_pos], skippingoutput_posbytes and potentially writing out-of-bounds or corruptingunrelated data.
Bug 2 — inflated returned
nout.The caller adds the returned
noutto its ownoutlenaccumulator.Returning
output_pos + bytes_writteninstead ofbytes_writtenmadeoutlengrow too fast, so every subsequent call would pass a slicestarting too far along in the backing buffer.
The DFA counterpart (
read_record_dfa) handles this correctly: itstarts
noutat0, computes field-end positions asself.output_pos + nout, and updatesoutput_posasself.output_pos + nout. This PR makes the NFA follow the samepattern (three-line change).
How to reproduce
The bug is only reachable when the hidden
nfa(true)builder option isset, so existing tests never hit the
OutputFullresume path. The addedtest calls
read_recordtwice for a single record using a 3-byte outputbuffer for a 5-byte first field:
Scope
The NFA is a debug/testing path (
#[doc(hidden)]builder option,off by default). The default DFA path is unaffected.