Skip to content

Close the out-of-bounds hole in ReadBytes/WriteBytes#74

Merged
CoreyRDean merged 2 commits into
developfrom
fix/readbytes-writebytes-bounds
May 30, 2026
Merged

Close the out-of-bounds hole in ReadBytes/WriteBytes#74
CoreyRDean merged 2 commits into
developfrom
fix/readbytes-writebytes-bounds

Conversation

@CoreyRDean
Copy link
Copy Markdown
Collaborator

Non-technical summary

ReadBytes/WriteBytes (which transfer raw bytes between a memory bank and a file/socket stream) trusted the byte count you handed them. A negative count — or a negative offset with a compensating count — slipped past the single bounds check and drove an out-of-bounds memory read/write from ordinary Blitz code: an arbitrary read/write primitive. This PR closes that hole using the same guard pattern the project already applied to CopyBank. Advances INTENT's "no silent corruption — bounds checks always worth doing."

Technical summary

bbReadBytes/bbWriteBytes in src/blitzrc/bbruntime/bbbank.cpp validated only the composite debugBank(b, offset+count-1) index — no count<=0 guard, no independent start-offset check:

  • ReadBytes(bank, s, 10, -5) → composite index 4 is in range → passes → s->read(data+10, -5); the negative count widens to a huge std::streamsize → massive OOB write into the bank.
  • ReadBytes(bank, s, -100, 200) → composite index 99 is in range → passes → reads into data-100 → OOB underflow.

bbCopyBank (Round-4 audit) already fixed this exact class with if(count<=0) return; + independent start and end debugBank checks. ReadBytes/WriteBytes were the unfinished tail (they still carried the old dead //if(debug){ scaffolding). This PR mirrors CopyBank's guards in both functions and removes the dead comments. In debug builds a bad index raises a clean Blitz RuntimeError; otherwise the call no-ops and returns 0. Valid positive, in-range calls are byte-identical.

Acceptance criteria + results

  • WriteBytes/ReadBytes with count <= 0 return 0 without touching the stream — verified: WriteBytes(..., -1) leaves the output FileSize = 0; ReadBytes(..., -1) leaves a sentinel-filled bank unchanged; both blocks complete (no crash) — the regression guard, since pre-fix the negative count drove an OOB transfer.
  • Happy-path round-trip (bank → file via WriteBytes, file → bank via ReadBytes) reproduces the bytes exactly.
  • Existing BankBoundsTest.bb (CopyBank) + full test.bat green; local build 0 errors; corpus sweep unaffected.

Trade-offs, deferred follow-ups

  • Pile-on note (disclosed): the prior iteration (Bounds-check the BBList API: bad indices fail cleanly instead of via UB #73) was BBList bounds-checking; this is another bounds fix. I weighed the loop's anti-pile-on bias and chose impact — a reachable arbitrary OOB read/write outranks the heuristic, and this is a different module/function family completing a committed audit. Next iteration will deliberately steer to a different area (diagnostics/DevEx).
  • Out of scope: defense-in-depth negative-size guards inside bbStream::read/write; CopyBank/Peek/Poke (already correct).
  • Deferred (confirmed): seTranslator missing-break (mislabels native crashes); human-readable compiler diagnostics (source line + caret, blitzide-gated); qstreambuf new[]/delete mismatch (stdutil.cpp).

🤖 Generated with Claude Code

CoreyRDean and others added 2 commits May 29, 2026 21:16
bbReadBytes/bbWriteBytes passed a caller-supplied count straight to the stream
layer after only a single composite debugBank(b, offset+count-1) check, with no
count<=0 guard and no independent start-offset check. Two arbitrary OOB memory
primitives reachable from pure Blitz code:
- ReadBytes(bank, s, 10, -5): composite index 4 is in range -> passes -> a negative
  count widens to a huge std::streamsize in the stream read -> massive OOB write.
- ReadBytes(bank, s, -100, 200): composite index 99 is in range -> passes -> reads
  into data-100 -> OOB underflow.

This is the same audit class bbCopyBank already fixed (count<=0 + independent start
and end checks); ReadBytes/WriteBytes were the unfinished tail and still carried the
old dead //if(debug){ scaffolding. Mirror CopyBank's guards in both functions and
drop the dead comments. Valid (positive, in-range) calls are byte-identical.
Advances INTENT's "no silent corruption / bounds checks always worth doing" goal.

tests/BankBoundsTest.bb: add a WriteBytes->file->ReadBytes round-trip plus
negative-count no-op cases for both (WriteBytes(-1) leaves FileSize 0; ReadBytes(-1)
leaves a sentinel-filled bank unchanged). The blocks completing is the regression
guard, since pre-fix the negative count drove an out-of-bounds transfer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root cause (two OOB primitives), the CopyBank idiom mirrored, the pile-on tradeoff
(impact over the anti-pile-on heuristic; steer to diagnostics/DevEx next), and
deferred follow-ups (seTranslator, compiler diagnostics, qstreambuf new[]/delete).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@CoreyRDean CoreyRDean requested a review from a team as a code owner May 30, 2026 02:18
@CoreyRDean CoreyRDean added the bug Something isn't working label May 30, 2026
@CoreyRDean CoreyRDean merged commit 8219714 into develop May 30, 2026
4 checks passed
@CoreyRDean CoreyRDean deleted the fix/readbytes-writebytes-bounds branch May 30, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant