Skip to content

csv-core: fix NFA read_record resumption after OutputFull - #430

Open
maxtaran2010 wants to merge 1 commit into
BurntSushi:masterfrom
maxtaran2010:fix/nfa-output-pos-off-by-one
Open

csv-core: fix NFA read_record resumption after OutputFull#430
maxtaran2010 wants to merge 1 commit into
BurntSushi:masterfrom
maxtaran2010:fix/nfa-output-pos-off-by-one

Conversation

@maxtaran2010

Copy link
Copy Markdown

What this fixes

read_record_nfa initialised its local nout counter to
self.output_pos instead of 0. When the function is called a second
time for the same record (because the output buffer was too small and
OutputFull was 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 the
received slice is the right place to start writing. Starting nout at
output_pos made the NFA write at output[output_pos], skipping
output_pos bytes and potentially writing out-of-bounds or corrupting
unrelated data.

Bug 2 — inflated returned nout.
The caller adds the returned nout to its own outlen accumulator.
Returning output_pos + bytes_written instead of bytes_written made
outlen grow too fast, so every subsequent call would pass a slice
starting too far along in the backing buffer.

The DFA counterpart (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 PR makes the NFA follow the same
pattern (three-line change).

How to reproduce

The bug is only reachable when the hidden nfa(true) builder option is
set, so existing tests never hit the OutputFull resume path. The added
test calls read_record twice for a single record using a 3-byte output
buffer for a 5-byte first field:

let input = b"hello,world\n";
let mut rdr = ReaderBuilder::new().nfa(true).build();

let (res1, nin1, nout1, _) = rdr.read_record(input, &mut [0u8; 3], ends);
// res1 == OutputFull, nout1 == 3, out[..3] == "hel"

let (res2, _, nout2, _) = rdr.read_record(&input[nin1..], &mut out2, ends);
// With the bug:  nout2 == 10, out2[..10] == "\0\0\0loworld" (wrong!)
// After the fix: nout2 == 7,  out2[..7]  == "loworld" (correct)

Scope

The NFA is a debug/testing path (#[doc(hidden)] builder option,
off by default). The default DFA path is unaffected.

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>
Copilot AI review requested due to automatic review settings July 9, 2026 21:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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