[Bughunt Phase 2] Memory safety: SPC binary-file decoder has unchecked file-controlled lengths and unbounded table fills

[grzanka]

Found during a full bughunt/architecture review. These are code-read findings (inspected, not executed against a live .spc/data file — I didn't have one on hand) in the parsers that read external, potentially untrusted files (treatment-planning SPC spectra, stopping-power CSV data). Flagging with that caveat, but the pattern is consistent enough across call sites that it warrants a dedicated pass rather than one-off patches.

1. decomposeStructInto{String,Double,Integer} trust a file-supplied length with no bound

src/AT_SPC.c:88-110:

void decomposeStructIntoDouble( const int32_t content[], double * value, int * length ){
    (*length) = content[1];          // length comes straight from the file
    memcpy( value, content+2, *length);
}

Callers pass single stack scalars as the destination — e.g. decomposeStructIntoInteger(content, &numberOfDepthSteps, &length) at src/AT_SPC.c:292, where numberOfDepthSteps is one uint64_t on the stack. There are ~20 call sites following this pattern (lines 147, 158, 170, 202, 207, 211, 214, 217, 221, 245, 253, 259, 265, 271, 277, 282, 287, 292, 302, 306, 311, 334, 339, 344, 362, 372, 378). A corrupted or crafted .spc file can set content[1] to an arbitrary length and overflow whatever fixed-size destination was passed — for the Integer/Double variants, a stack write.

2. decomposeStructIntoString never writes through to the caller (and leaks)

src/AT_SPC.c:88-94:

void decomposeStructIntoString( const int32_t content[], char * string, int * length ){
    (*length) = content[1];
    string = (char*)calloc(sizeof(char),*length);   // reassigns the LOCAL parameter
    memcpy( string, content+2, *length);
}

string is a local copy of the pointer; reassigning it does nothing for the caller, and the freshly-calloc'd buffer is immediately leaked on every call. Concretely, in AT_SPC_decompose_header (src/AT_SPC.c:202-210) targetName/projectileName are declared as char * targetName = NULL; and passed in — they come back NULL regardless of file content, so the function falls back to hardcoding *material_no = Water_Liquid and *particle_no = 6012 (with a // TODO write converter from targetName to material_no comment acknowledging the gap). An SPC file describing, say, protons in PMMA is silently reinterpreted as carbon-12 in water — a correctness bug with the same root cause as the memory-safety one.

3. Unvalidated index used as a memcpy source-array index

src/AT_SPC.c:374:

uint64_t lSRef;
decomposeStructIntoInteger(content, &lSRef, &length);
memcpy( (*E_MeV_u)+index , binPointersE[lSRef], nE * sizeof(double));

lSRef is read straight from the file and used to index binPointersE/binPointersDE (sized to numberOfSpecies, src/AT_SPC.c around line 311) with no range check.

4. Partially-initialized locals read unconditionally

• double tmp[2] (src/AT_SPC.c:328): only tmp[0] is reliably populated by decomposeStructIntoDouble's length-driven copy; (int)(tmp[1]) is read immediately after at line 331 to compute particle_no_spc.
• tempBins (src/AT_SPC.c:359-368) is only populated inside an if (length != (nE+1)*sizeof(double)) branch; on the else (mismatch) path, E_MeV_u[i]/DE_MeV_u[i] for that step are left with whatever was in the caller's buffer, and no error is signaled to the top-level caller.
• Debug-only fopen at src/AT_SPC.c:76 has no NULL check before the subsequent fread.

5. Two more allocation sites fill fixed-capacity buffers with no bound check

• src/AT_SPC.c:543 and :645: struct spc_pair *table = calloc(1000, sizeof(struct spc_pair)), then the directory-scan loop does table[index] = tmp; index++ with no check against 1000. A directory containing more than 1000 .spc files (not unusual for a full SPC spectrum set) silently corrupts the heap.
• src/AT_StoppingPowerDataFromFile.c:11,45-66: Z/E/S are calloc(MAX_NUMBER_OF_LINES=10000, ...), and the read loop's i++ (line ~64) is never checked against that bound — a data file with more than 10000 rows overflows three heap buffers back to back. The loop's error-return path (src/AT_StoppingPowerDataFromFile.c:59, on a malformed line) also returns without fclose(CSV) or freeing Z/E/S — a leak on every malformed-input error, not just the overflow case.

Proposed work

• Give decomposeStructInto{String,Double,Integer} an explicit destination-capacity parameter; validate the file-supplied length against it and return an error (propagated up through AT_SPC_decompose_header/AT_SPC_decompose_data) instead of memcpy-ing blind
• Fix decomposeStructIntoString to write through a char ** (or take a pre-sized buffer) so the caller actually receives the string, and implement the target/projectile-name → material_no/particle_no converters (closing the TODO and the silent-carbon-in-water bug)
• Bounds-check lSRef against numberOfSpecies before indexing binPointersE/binPointersDE
• Either grow the struct spc_pair table dynamically or reject/report once the 1000-entry capacity is exceeded, in both AT_SPC.c call sites
• Either grow Z/E/S dynamically or reject once MAX_NUMBER_OF_LINES is exceeded in AT_StoppingPowerDataFromFile.c; fix the leak-and-no-fclose error path
• Add a small fuzz target (libFuzzer or AFL++, whichever is lighter to wire into the existing CMake/CI) over AT_SPC_decompose_data/AT_SPC_decompose_header — this is a binary parser over externally-sourced files and warrants it
• Add regression tests using a small set of valid and deliberately-truncated/corrupted .spc fixtures once (2) is fixed enough to construct them

Acceptance criteria

• No decomposeStructInto* call can write past a caller-supplied destination's real capacity
• AT_SPC_decompose_header returns the actual material/particle from the file, not a hardcoded default
• Directory scans and CSV loads reject or grow past their fixed capacities instead of overflowing
• A fuzz target exists for the SPC decoder and runs (even if only ad hoc, not necessarily in CI yet)