Fix substrate csv out-of-bounds reads and revive the headerless path on my-physicell - #69
Merged
drbergman merged 1 commit intoAug 21, 2026
Conversation
…quire well-formed rows (#66) * 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
drbergman
added a commit
that referenced
this pull request
Aug 21, 2026
#69 gave the substrate reader a parser that returns the 1-based index of the first malformed field and yields NaN for an omitted entry. That NaN sentinel is exactly what the parallel is_missing vector existed to carry, so the dirichlet reader can use the same parser and both dirichlet_csv_to_vector and the store_dirichlet_csv_entry helper of the previous revision of this branch go away. Rather than copy the header handling a third time, the two loaders now share it: resolve_substrate_csv_columns reads the optional "x,y,z,<name>,..." header, applies the headerless "first n densities" convention, and leaves the stream on the first data row. The two readers differ only in what a parsed row does to the microenvironment -- the substrate reader resolves an omitted entry to 0, the dirichlet reader leaves that voxel-substrate pair alone -- and in the word their diagnostics use. Sharing it also carries #69's fixes onto the dirichlet path, which had the same two bugs its substrate counterpart did: - "if (i<3) {continue;}" jumped past the "i++" below it, so substrate_indices came out empty on a headerless file. - the reopened "std::ifstream file(filename, ...)" shadowed the enclosing stream, which had just been closed, so the row loop read a closed stream. A headerless dcs.csv therefore set no dirichlet conditions at all and said nothing about it. Verified on the dirichlet_from_file sample: before this change a headerless dcs.csv produced a byte-identical final microenvironment to disabling the file entirely; after it, it matches the headered file. Blank lines are skipped rather than read as a row of zeroes, a row whose column count disagrees with the header is a hard error, and every diagnostic names the line and column it rejected. The out-of-bounds write in dirichlet_csv_to_vector, which had no bound on ind, disappears with the function. The stock sample is unaffected: its dcs.csv leans on empty fields meaning "leave this pair alone" on nearly every row, and it produces byte-identical initial and final microenvironments before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
drbergman
added a commit
that referenced
this pull request
Aug 21, 2026
#69 gave the substrate reader a parser that returns the 1-based index of the first malformed field and yields NaN for an omitted entry. That NaN sentinel is exactly what the parallel is_missing vector existed to carry, so the dirichlet reader can use the same parser and both dirichlet_csv_to_vector and the store_dirichlet_csv_entry helper of the previous revision of this branch go away. Rather than copy the header handling a third time, the two loaders now share it: resolve_substrate_csv_columns reads the optional "x,y,z,<name>,..." header, applies the headerless "first n densities" convention, and leaves the stream on the first data row. The two readers differ only in what a parsed row does to the microenvironment -- the substrate reader resolves an omitted entry to 0, the dirichlet reader leaves that voxel-substrate pair alone -- and in the word their diagnostics use. Sharing it also carries #69's fixes onto the dirichlet path, which had the same two bugs its substrate counterpart did: - "if (i<3) {continue;}" jumped past the "i++" below it, so substrate_indices came out empty on a headerless file. - the reopened "std::ifstream file(filename, ...)" shadowed the enclosing stream, which had just been closed, so the row loop read a closed stream. A headerless dcs.csv therefore set no dirichlet conditions at all and said nothing about it. Verified on the dirichlet_from_file sample: before this change a headerless dcs.csv produced a byte-identical final microenvironment to disabling the file entirely; after it, it matches the headered file. Blank lines are skipped rather than read as a row of zeroes, a row whose column count disagrees with the header is a hard error, and every diagnostic names the line and column it rejected. The out-of-bounds write in dirichlet_csv_to_vector, which had no bound on ind, disappears with the function. The stock sample is unaffected: its dcs.csv leans on empty fields meaning "leave this pair alone" on nearly every row, and it produces byte-identical initial and final microenvironments before and after. 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.
Cherry-picks the substrate initial-condition csv hardening (#66) onto
my-physicell.my-physicellalready carries the omitted-voxels feature, which arrived via #37. That change deleted the strict post-loop check requiring a csv to cover every voxel exactly — but the bounds checks that should have replaced it live onsubstrate-csv-omit-voxels, and the two lines have diverged, so they were never going to arrive here on their own.my-physicelltherefore has the half that removes the old backstop and none of the half that replaces it, which makes it strictly worse off thandevelopment:data[0..2]anddata[ci + 3]were indexed without consultingdata.size()developmentat least fails loudly here (Wrong number of voxels supplied ... Found: 0); deleting the coverage check removed even that signalstrtod'sendptrwas discarded. That shifts the row's column count, which is what turnsNAandinfinto an out-of-bounds read and lets1.5abcland as a plausible densitynearest_voxel_indexclamps toThe headerless path had never worked, for two reasons that only show up together: a loop counter that never incremented left the substrate index list empty, and a reopened
ifstreamshadowed the enclosing stream — which had just been closed — so the row loop read a closed stream and processed nothing.-Wshadowflags the second one.Rows are now required to be well formed, with every diagnostic naming the offending line and the column where relevant. Omitted entries (an empty field,
0,0,0,,3.5) still resolve to 0, which keeps the csv the single source of truth for any voxel it names.Conflict resolution worth a look. #37 added a Dirichlet-from-csv loader that is structurally a near-copy of the substrate loader, so git aligned my rewritten substrate functions against the Dirichlet ones and produced ten tangled hunks. Rather than resolve those, I spliced
my-physicell's substrate block out and the hardened block in, leaving the Dirichlet code untouched.my-physicellhad made no functional change to the substrate loader — only trailing whitespace and one stale comment — so nothing was dropped.substrate_csv_to_vectorhas exactly one caller after this; the Dirichlet path uses its owndirichlet_csv_to_vectorand is unaffected.Not fixed here:
dirichlet_csv_to_vectorhas the same bug class, and worse — it writesis_missing[ind]anddata[ind]with no bound onind, so a Dirichlet row with extra columns is an out-of-bounds write; its row-length check runs after those writes, so it cannot prevent them. It also usesstd::stod, which throws on a non-numeric field with nothing catching it, and its warning has the samenumber_of_voxels()/number_of_densities()copy-paste as the substrate one had. Separate change, since it is a separate feature.