Skip to content

Commit eb4d1a0

Browse files
brianeggeclaude
andcommitted
Stop the scanner writing past its backtracking buffer on a long token (Covered by dco/Brian_Egge.md)
OSS-Fuzz testcase 6480698828193792 is recorded as an UNKNOWN READ inside unique_refc_map's hash table, security medium, not reliably reproducible and with no minimized reproducer. Replayed under ASan it reproduces every time, and it is not a read: ERROR: AddressSanitizer: heap-buffer-overflow WRITE of size 4 at 0x531000038808 #0 yylex() hexpr.lex.C:1101 0x531000038808 is located 0 bytes after 65544-byte region allocated by yyalloc() at hexpr.lex.C:1035 That region is the scanner's backtracking state stack. flex allocates it once, at (YY_BUF_SIZE + 2) * sizeof(yy_state_type) = 65,544 bytes, and never grows it, while pushing one entry for every character it scans while matching a token. yy_scan_string takes a buffer of whatever length it is handed, and nothing relates the two, so a token longer than 16,386 characters writes four bytes past the end of the buffer for every further character it scans. It takes nothing exotic: 20,000 letters is one identifier and does it, 16,000 is fine. That also explains the crash OSS-Fuzz recorded. The overflow runs off the end of a heap block into whatever is next, and the failure surfaces later, wherever that memory is used -- for that run, in a hash table belonging to the type memo. Hence a read in a place with no apparent connection to the input, and hence "not reliably reproducible". The buffer exists only to support trailing context. One rule used it: the indentation rule needs a character or two of lookahead to tell an indented definition from a comment, and asked for it as `/([^ \t\n/]|"/"[^*/])`. Head and trailing part are both variable length, which is the case that makes flex carry the state stack. There is no REJECT in this scanner otherwise. So match the lookahead and give it back, rather than asking flex to look ahead: two rules and yyless() in place of one rule with trailing context. The two rules mirror the two alternatives exactly, and flex counts head plus trailing length when choosing the longest match, so the same input still selects the same rule. With no trailing context left anywhere, flex emits no backtracking buffer at all -- references to yy_state_buf in the generated scanner go from nine to none -- and a token is bounded only by the input. An old bug, not a regression: the rule has had trailing context since the scanner was written. hexpr.lex.C is regenerated (the flex step of pgen.sh) with GNU flex 2.6.4, the version named in the checked-in file. The diff is large for two reasons: the DFA tables really do change, and the checked-in file came from a slightly different 2.6.4 build, so regenerating even the unmodified hexpr.l accounts for about 800 lines of it on its own. Nothing in the tree pins the generator. Verified with an ASan build on Linux (clang 18, -DUSE_ASAN_AND_UBSAN=ON): the reported testcase and a 20,000 character identifier, string and regex literal all crash before this change and are clean after it. The full test suite passes, as do the 133 rosetta examples. test/Parse.C gains two tests. TokensLongerThanTheScanBufferAreSafe reads tokens past the old limit; note that it only detects the overflow in a sanitized build, since an unsanitized one writes past the block and carries on. IndentedDefinitionsStillRead covers the behaviour the rewritten rules carry: members of a class or instance are found by their indentation and an indented comment is not one of them. test/Objects.C covers that too, but only on a non-clang build, so nothing was checking it here. An indented block comment inside an instance body is rejected identically before and after this change ("unexpected indent, expecting id or ("), which is behaviour this change deliberately leaves alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016rGT4C394qeh2DBQhqy3Tb
1 parent a340567 commit eb4d1a0

3 files changed

Lines changed: 590 additions & 556 deletions

File tree

lib/hobbes/read/pgen/hexpr.l

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,24 @@ void popIndent() {
9494
}
9595
}
9696

97+
// Give back the last n characters of the current match.
98+
//
99+
// The indentation rules below need one or two characters of lookahead to tell
100+
// an indented line from a comment, and they used to ask flex for it as
101+
// trailing context. Variable trailing context makes flex carry a backtracking
102+
// state stack -- one entry per character scanned while matching a token -- in
103+
// a buffer it allocates once at (YY_BUF_SIZE + 2) states and never grows, so
104+
// scanning a token longer than that wrote past the end of it. Matching the
105+
// lookahead and handing it back leaves no rule with trailing context, and flex
106+
// emits no such buffer at all, so the length of a token is bounded only by the
107+
// input. yyless rewinds yyleng, which YY_USER_ACTION has already used, so the
108+
// end of the token's location is restated here.
109+
#define pushBackLookahead(n) \
110+
do { \
111+
yyless(yyleng - (n)); \
112+
yylloc.last_column = yylloc.first_column + yyleng - 1; \
113+
} while (false)
114+
97115
std::string* identifier(const char* b, const char* e) {
98116
if ((b+1) == e && *b == '_') {
99117
return hobbes::autorelease(new std::string(hobbes::freshName()));
@@ -120,7 +138,8 @@ std::string* identifier(const char* b, const char* e) {
120138

121139
\n { yycolumn = 1; }
122140
[ \t\r\f] { }
123-
"\n"[ \t]+/([^ \t\n/]|"/"[^*/]) { yycolumn = 2; if (wantIndent()) { return TINDENT; } }
141+
"\n"[ \t]+[^ \t\n/] { pushBackLookahead(1); yycolumn = 2; if (wantIndent()) { return TINDENT; } }
142+
"\n"[ \t]+"/"[^*/] { pushBackLookahead(2); yycolumn = 2; if (wantIndent()) { return TINDENT; } }
124143
"\n " { yycolumn = 2; if (wantIndent()) { return TINDENT; } }
125144
"\r " { if (wantIndent()) { return TINDENT; } }
126145
"\n\t" { yycolumn = 2; if (wantIndent()) { return TINDENT; } }

0 commit comments

Comments
 (0)