Validate serialized scanner state before restoring it - #5
Open
tarebyte wants to merge 2 commits into
Open
Conversation
`deserialize` read the serialized scanner state without any bounds checking and only sanity-checked the result with an `assert` that is compiled out of release builds. Any buffer that did not exactly match the layout `serialize` produces was restored anyway, and the 32-bit heredoc word length was passed straight to `memcpy`, so a malformed or truncated buffer could read gigabytes past the end of the caller's buffer. Bounds-check every field against the remaining input, reject out-of-range literal token types, and reset the scanner to its empty state whenever the buffer does not describe a complete, exactly-consumed state. A stale or corrupt buffer now degrades to a fresh scan instead of corrupting memory. Add a Rust test that feeds pseudo-random buffers through deserialize/serialize; it crashes with SIGBUS against the previous implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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.
Problem
deserialize()insrc/scanner.cread the serialized scanner state without bounds checking any field. Tree-sitter hands back whatever buffer it was given, so those bytes are untrusted input.The worst case is the heredoc record, which carries a 32-bit end-word length that went straight to
array_reserveandmemcpywith no comparison against the remaining buffer:The only size check was
assert(size == length)at the very end — after every read had already happened.Change
Bounds-checks every field read: the leading byte, each 5-byte literal record, the heredoc fixed fields, the length-prefixed end word, and the trailing bytes. Any inconsistency calls
reset()and returns, so the scanner starts from an empty state rather than restoring garbage.Literal records are also checked against the token types that can actually reach the literal stack. Those are the delimited-literal opening tokens, which form one contiguous run in
TokenType; rejecting only out-of-enum values would still admit records describing states the scanner cannot produce, such as a literal whose type isLINE_BREAKorNONE.Scope of the validation
This rejects buffers that are structurally impossible: wrong length, unconsumed trailing bytes, an end word longer than the buffer, or a literal type that cannot occur. It does not verify the remaining field values — delimiters, nesting depth, and the boolean flags are accepted as-is, so a hand-crafted buffer of the right shape can still restore a nonsensical-but-harmless state. The goal here is memory safety, not full state validation.
Validation
test_external_scanner_rejects_malformed_statedrives 20,000 pseudo-random buffers throughdeserializeand re-serializes each one. Againstmasterit fails, in both debug and release, withAssertion failed: (size == length) ... signal: 6, SIGABRT. It passes on this branch.Note that the assertion is what surfaces the corruption here —
cargodoes not defineNDEBUG, so it is present in release builds too. A consumer that compiles the scanner withNDEBUGloses it and gets the out-of-bounds read silently, which is the case this PR is really about.Building
masterwith-fsanitize=address,undefinedand running the same deterministic 20,000-buffer sequence reportsERROR: AddressSanitizer: stack-buffer-overflow ... in deserialize. The identical run on this branch is clean.The read length depends on the bytes that land in the
word_lengthfield and varies per seed. Most stop at the first byte past the buffer; the largest observed across the seeds tried was a 2,985,668,234-byte read, which is simply the old code trusting the fulluint32_t.test_external_scanner_round_trips_well_formed_stateandtest_external_scanner_rejects_impossible_literal_typecall the scanner's serialize/deserialize entry points directly. They confirm that a valid string-literal state and a valid started-heredoc state come back byte-identical, that every acceptable literal type round-trips, and thatLINE_BREAK,COMMENT,UNINTERPRETEDandNONEreset instead. These are the tests that show validation does not discard real state.332 corpus parses and 5 highlight files, zero failures. All 4,437
examples/ruby_specfiles produce byte-identical trees tomaster. Both are regression checks: a from-scratch parse callsdeserializewith a zero length, so neither exercises the changed code.CI already runs the Rust tests (
test-rust: truein.github/workflows/ci.yml).References
Hardens the code path in tree-sitter#269, where the same assertion fires while parsing a heredoc, and covers the malformed-state handling proposed in tree-sitter#297.
Merge note
This and #12 both add tests to the same module in
bindings/rust/lib.rsand will conflict; whichever lands second needs the other's test block re-applied.