fix(wire): hoist read_cstring/parse_error_fields' nested recursive fn to top-level - #2
Merged
Merged
Conversation
`Inner`/`Left`/`Right` (JoinKind) and `Asc`/`Desc` (SortDir) are declared by both `Ast` and `DataFrame`, so under current march they are ambiguous wherever both modules are in scope — 17 errors across build.march and compile.march that any consumer inherits (found compiling bastion). Every site here means the SQL AST's own constructors, so all qualify to `Ast.*`. Same treatment as f11be60 did for the `Expr.*` collisions, and the doctests in build.march are qualified along with the code they document so they keep running. `forge check`, `forge build` and `forge lint --strict` are clean against march main-a5c3dc70. `forge test` does not yet compile on that toolchain, for two reasons that are not this change: six `Module Depot.* not found` resolution failures, and test/eq_impls.march's universal `impl Eq(a)` now colliding with a built-in implementation (coherence). Both are unchanged by this commit and want separate fixes.
… to top-level Both had a local `fn go(...) do ... end` defined and immediately called inside their outer function, closing over the buffer `b`. On the compiled backend this corrupts the recursive closure's own first parameter: traced with debug prints, `go`'s `i` parameter read back as a garbage ~4.3 billion value (looks like a raw pointer) on its very first call, `go(pos, Nil)`, where `pos` was 0 — never reaching the intended base case, so the very next `Bytes.get(b, i)` call panics with "index out of bounds". The interpreter always evaluated these correctly; this is compiled-backend only, same class of bug already worked around in conduit's cron_parser.march (nested recursive closure, not this specific shape). Symptom: any multi-field ErrorResponse/NoticeResponse (parse_error_fields) or any C-string field beyond the first (read_cstring, used by ParameterStatus, RowDescription, CommandComplete, SASL mechanism lists, and error-field parsing) intermittently panicked in a compiled Postgres client — reproduced via forgepm's test suite as a segfault-adjacent crash in `Bytes.get: index out of bounds` on every DB test that triggered a Postgres error response, and (before this) an incorrect duplicate-email error classification because the unique-violation error's message text never survived parse_error_fields intact. Fix: hoist both `go` functions to top-level `pfn`s (read_cstring_go, parse_error_fields_go) taking `b` as an explicit parameter instead of a closure capture. Purely mechanical — no behavior change, verified against forgepm's full 544-test suite (was 10 failures from this bug + 1 from the parse_error_fields half specifically, now 0).
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
1 participant
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.
Summary
read_cstringandparse_error_fieldseach defined a localfn go(...) do ... endclosing over the bufferb, called immediately asgo(pos, ...). On the compiled backend this corrupts the closure's own first parameter — traced with debug prints,go'siread back as a garbage ~4.3 billion value (looks like a raw pointer) on its very first call, where the caller passedpos = 0. The very nextBytes.get(b, i)then panics with "index out of bounds". The interpreter always evaluated these correctly — this is compiled-backend only, same class of miscompile already worked around in conduit'scron_parser.march.Symptom (found via forgepm's test suite): any multi-field
ErrorResponse/NoticeResponseor any C-string field beyond the first (ParameterStatus,RowDescription,CommandComplete, SASL mechanism lists, error-field parsing) intermittently panicked in a compiled Postgres client. 10 of forgepm's DB tests crashed withBytes.get: index out of bounds, and duplicate-email/username classification silently fell back to a generic"db_error"because the unique-violation message text never survivedparse_error_fieldsintact.Fix
Hoist both
gofunctions to top-levelpfns (read_cstring_go,parse_error_fields_go) takingbas an explicit parameter instead of a closure capture. Purely mechanical, no behavior change.Test plan
forge check— clean