Fix substrate csv out-of-bounds reads, revive the headerless path, require well-formed rows - #66
Merged
drbergman merged 2 commits intoAug 19, 2026
Conversation
get_row_from_substrate_initial_condition_csv indexed data[0..2] and
data[ci + 3] without consulting data.size(). Both reads are out of
bounds for a malformed row:
- substrate_csv_to_vector always emits a final field, so a blank line
parses to a single 0.0 and data[1]/data[2] read past the end.
- substrate_indices is sized from the header's column count, so a row
with fewer columns than the header runs data[ci + 3] off the end.
Reproduced with libc++ bounds checking on a 4-voxel microenvironment: a
trailing blank line, a whitespace-only line, a CRLF blank line, a
2-column row, and a row supplying 1 of 3 header substrates all abort
with "vector[] index out of bounds" before this change and are handled
cleanly after it. Note these reads are not new here -- development
crashes on the same five inputs. Its coverage check ran after the read
loop, so it never guarded them.
Blank lines are now skipped, matching load_cells_csv_v1 and
process_csv_v2_line, and a short row is a hard error rather than a
silent partial write. Ordering the guards ahead of the existing warning
also stops data.size() - 3 from underflowing on a short row.
Also correct that warning: it reported number_of_voxels() where it
means number_of_densities(), and the block comment still described the
one-row-per-voxel, no-header format this branch replaced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The headerless branch of load_initial_conditions_from_csv could never
have worked. Two bugs had to be fixed together to see either one:
- "if (i<3) {continue;}" jumped past the "i++" below it, so i never
advanced and substrate_indices came out empty.
- the reopened "std::ifstream file(filename, ...)" shadowed the
enclosing stream, which had just been closed, so the row loop read a
closed stream and processed no rows. -Wshadow flags this.
A headerless csv therefore loaded nothing. On development that at least
failed loudly, because the deleted coverage check saw voxel_set.size()
== 0 and exited; without it the run continued silently on the config
file's uniform initial conditions. Reading the first row's column count
and rewinding the stream fixes both, and the row loop now counts lines
so every diagnostic can name the row it rejected.
Rows are now held to being well formed rather than parsed as far as
they go:
- a field must be empty or a complete finite number. strtod's endptr
was discarded, so "NA" and "1.5abc" silently became 0 and "inf"
became a density.
- a row must have exactly the column count the header (or the first
row) established. Extra columns were silently dropped.
- x, y and z must all be present, so ",,,," no longer resolves to the
origin.
- a position must lie inside the domain, since nearest_voxel_index
clamps and would otherwise snap a typo onto an edge voxel.
- a header may not name the same substrate twice, and must name at
least one.
- an empty file is an error rather than a header sniff on nothing.
An omitted entry now travels as NaN instead of 0, which is what lets the
row loop tell "the user left this blank" from "the user asked for zero".
It still resolves to 0, as before; the distinction only makes the checks
above possible.
The header sniff also no longer indexes line.c_str()[2] and [4] without
knowing they exist -- a first line of "x" read past the end. It splits
the row and compares fields instead, which also makes the sniff
whitespace tolerant.
Verified against real BioFVM on a 2x2x1 microenvironment with libc++
bounds checking: 36 cases covering header/headerless, subsets,
reordering, whitespace, CRLF, blank lines, and every rejection above.
All 36 behave as intended with no out-of-bounds reads. Headerless files
load correctly for the first time.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
drbergman
added a commit
that referenced
this pull request
Aug 21, 2026
…quire well-formed rows (#66) (#69) * Bounds-check substrate IC csv rows before indexing them get_row_from_substrate_initial_condition_csv indexed data[0..2] and data[ci + 3] without consulting data.size(). Both reads are out of bounds for a malformed row: - substrate_csv_to_vector always emits a final field, so a blank line parses to a single 0.0 and data[1]/data[2] read past the end. - substrate_indices is sized from the header's column count, so a row with fewer columns than the header runs data[ci + 3] off the end. Reproduced with libc++ bounds checking on a 4-voxel microenvironment: a trailing blank line, a whitespace-only line, a CRLF blank line, a 2-column row, and a row supplying 1 of 3 header substrates all abort with "vector[] index out of bounds" before this change and are handled cleanly after it. Note these reads are not new here -- development crashes on the same five inputs. Its coverage check ran after the read loop, so it never guarded them. Blank lines are now skipped, matching load_cells_csv_v1 and process_csv_v2_line, and a short row is a hard error rather than a silent partial write. Ordering the guards ahead of the existing warning also stops data.size() - 3 from underflowing on a short row. Also correct that warning: it reported number_of_voxels() where it means number_of_densities(), and the block comment still described the one-row-per-voxel, no-header format this branch replaced. * Revive the headerless csv path and require well-formed rows The headerless branch of load_initial_conditions_from_csv could never have worked. Two bugs had to be fixed together to see either one: - "if (i<3) {continue;}" jumped past the "i++" below it, so i never advanced and substrate_indices came out empty. - the reopened "std::ifstream file(filename, ...)" shadowed the enclosing stream, which had just been closed, so the row loop read a closed stream and processed no rows. -Wshadow flags this. A headerless csv therefore loaded nothing. On development that at least failed loudly, because the deleted coverage check saw voxel_set.size() == 0 and exited; without it the run continued silently on the config file's uniform initial conditions. Reading the first row's column count and rewinding the stream fixes both, and the row loop now counts lines so every diagnostic can name the row it rejected. Rows are now held to being well formed rather than parsed as far as they go: - a field must be empty or a complete finite number. strtod's endptr was discarded, so "NA" and "1.5abc" silently became 0 and "inf" became a density. - a row must have exactly the column count the header (or the first row) established. Extra columns were silently dropped. - x, y and z must all be present, so ",,,," no longer resolves to the origin. - a position must lie inside the domain, since nearest_voxel_index clamps and would otherwise snap a typo onto an edge voxel. - a header may not name the same substrate twice, and must name at least one. - an empty file is an error rather than a header sniff on nothing. An omitted entry now travels as NaN instead of 0, which is what lets the row loop tell "the user left this blank" from "the user asked for zero". It still resolves to 0, as before; the distinction only makes the checks above possible. The header sniff also no longer indexes line.c_str()[2] and [4] without knowing they exist -- a first line of "x" read past the end. It splits the row and compares fields instead, which also makes the sniff whitespace tolerant. Verified against real BioFVM on a 2x2x1 microenvironment with libc++ bounds checking: 36 cases covering header/headerless, subsets, reordering, whitespace, CRLF, blank lines, and every rejection above. All 36 behave as intended with no out-of-bounds reads. Headerless files load correctly for the first time. --------- Co-authored-by: Claude Opus 5 <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.
Stacked on top of
substrate-csv-omit-voxels(MathCancer#386 upstream). Merge this into that branch first, then MathCancer#386 carries it.Two commits: the first closes out-of-bounds reads, the second revives the headerless path and tightens what counts as a well-formed row.
Out-of-bounds reads on malformed rows
get_row_from_substrate_initial_condition_csvindexeddata[0..2]anddata[ci + 3]without consultingdata.size().substrate_csv_to_vectoralways emits a final field, so a blank line parsed to a single value anddata[1]/data[2]read past the end;substrate_indicesis sized from the header, so a row shorter than the header randata[ci + 3]off the end.These reads are not new here —
developmentcrashes on the same inputs. Its coverage check ran after the read loop, so it never guarded them.The headerless path never worked
Two bugs that only show up together:
if (i<3) {continue;}jumped past thei++below it, sosubstrate_indicescame out empty.std::ifstream file(filename, ...)shadowed the enclosing stream, which had just been closed, so the row loop read a closed stream and processed zero rows.-Wshadowflags it.A headerless csv loaded nothing. This is where deleting the coverage check does bite: on
developmentthe emptyvoxel_settripped it and the run exited loudly; without it the run continues silently on the config file's uniform initial conditions. Fixed by taking the column count from the first row and rewinding the stream.Rows must now be well formed
Previously a row was parsed as far as it would go and the remainder ignored:
0,0,0,NA0,0,0,1.5abc0,0,0,inf,,,,strtod'sendptrwas being discarded, which is what let the first three through. An omitted entry now travels as NaN so the row loop can tell "left blank" from "asked for zero"; it still resolves to 0, exactly as before — the distinction is only what makes the checks above expressible.Every diagnostic now names the line, and the column where relevant.
Other cleanup
line.c_str()[2]and[4]without knowing they exist — a first line ofxread past the end. It splits the row and compares fields now, which also makes the sniff whitespace tolerant.static bool warning_issued; the column count is a property of the file, so the "supplying only the first n densities" warning is emitted once by the loader rather than from inside the row loop.find_density_indexwas called twice per header column.std::stringandstd::vector<int>by value, copying both per row.Verification
Built against real BioFVM on a 2x2x1 microenvironment with
-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG, so any surviving out-of-bounds read aborts. 36 cases: header and headerless, substrate subsets, reordered headers, whitespace, CRLF, blank lines, plus every rejection above.development, the same 5.BioFVM/,core/andmodules/compile; no new-Wall -Wextra -Wshadowwarnings.Worth a second opinion
load_cells_csv's empty token means "leave the value alone" instead. Now that omitted entries arrive as NaN, switching to match is a one-line change if you'd rather they agree.🤖 Generated with Claude Code