Skip to content

Commit b960bb9

Browse files
sjqtentaclesclaude
andcommitted
Fix Int.fromString overflow on untrusted parse paths (cross-compiler)
`case Int.fromString s of SOME|NONE` does not catch Overflow: MLton's 32-bit default int raises it past 2^31 (a crash) while Poly/ML's 63-bit int accepts up to 2^62 -- a crash and a cross-compiler divergence, the case-form of the earlier `valOf (Int.fromString ...)` sweep. Fixed on every untrusted-input parse path by parsing via `IntInf.fromString` with a fixed-literal signed-32-bit bounds check (or `IntInf` end-to-end for genuinely-64-bit domains like Redis/SQL integers), returning the site's documented failure (NONE / Err / documented exception) when out of range. Never `Int.maxInt` (NONE on Poly) or bare `handle Overflow` (only overflows on MLton). Re-vendored fixed dependencies to keep vendored copies byte-matching canonical. Verified: `make test` (MLton) and `make test-poly` (Poly/ML) both green with byte-identical harness output. Co-Authored-By: Claude Opus 4.8 <[email protected]>
1 parent e70f562 commit b960bb9

3 files changed

Lines changed: 54 additions & 6 deletions

File tree

README.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ byte-identical** under both [MLton](http://mlton.org/) and
2222
2323
## Status
2424

25-
- **114 assertions, green on MLton and Poly/ML**, with byte-identical output.
25+
- **117 assertions, green on MLton and Poly/ML**, with byte-identical output.
2626
- Validated against **real `git` fixtures** committed under
2727
[`test/fixtures/`](test/fixtures/), generated by the system `git` CLI
2828
([`generate.sh`](test/fixtures/generate.sh)) with pinned identities/dates so
@@ -176,7 +176,11 @@ end
176176
fully-resolved base object.
177177
- **32-bit-safe.** All binary reads stay below 2³¹ so MLton's default 32-bit
178178
`Int` never overflows; packs larger than 2 GiB (64-bit offsets) are rejected
179-
rather than silently mis-decoded.
179+
rather than silently mis-decoded. The loose-object header size (unbounded
180+
decimal ASCII) is parsed through arbitrary-precision `IntInf` and range-checked
181+
against the fixed 32-bit signed bound, so a corrupt or hostile oversized size
182+
is rejected as a clean `Git` error rather than raising `Overflow` on MLton
183+
(which would also diverge from Poly/ML's wider `int`).
180184
- **Malformed input raises `Git`.**
181185

182186
## Build & test

src/git.sml

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@
88
Everything is byte-string in / byte-string out and deterministic. Integers
99
stay small (MLton's default Int is 32-bit), so the binary readers avoid any
1010
value >= 2^31; 64-bit packfile offsets (packs > 2 GiB) are explicitly
11-
rejected rather than silently overflowing. *)
11+
rejected rather than silently overflowing. The one size field parsed from
12+
untrusted decimal ASCII (a loose object's header size) is parsed through
13+
arbitrary-precision IntInf and range-checked against the fixed 32-bit bound,
14+
so an oversized size is a clean `Git` rejection rather than an Overflow
15+
crash that would diverge between compilers. *)
1216

1317
structure Git :> GIT =
1418
struct
@@ -195,9 +199,22 @@ struct
195199
val nul = findChar (framed, #"\000", sp + 1)
196200
val () = if nul < 0 then raise Git "object: missing header terminator" else ()
197201
val sizeStr = String.substring (framed, sp + 1, nul - (sp + 1))
198-
val size = case Int.fromString sizeStr of
199-
SOME k => k
200-
| NONE => raise Git "object: malformed size"
202+
(* The header size is unbounded decimal ASCII, so a corrupt or hostile
203+
object can carry a value past 2^31. It is only ever compared to
204+
`String.size p` (a machine `int`, since no in-memory string can be
205+
larger), so it stays a bounded `int` -- but we must not let
206+
`Int.fromString` raise `Overflow` on MLton's 32-bit `int` (a crash that
207+
also diverges from Poly/ML's 63-bit `int`). Parse through
208+
arbitrary-precision `IntInf` and range-check against the FIXED 32-bit
209+
signed range, rejecting anything out of range as a clean `Git` error --
210+
identically on both compilers. *)
211+
val size =
212+
case IntInf.fromString sizeStr of
213+
NONE => raise Git "object: malformed size"
214+
| SOME k =>
215+
if k >= 0 andalso k <= 2147483647
216+
then IntInf.toInt k
217+
else raise Git "object: size out of range"
201218
val p = String.substring (framed, nul + 1, String.size framed - (nul + 1))
202219
val () = if String.size p <> size then raise Git "object: size mismatch" else ()
203220
in parseObject {typ = typ, payload = p} end

test/test_object.sml

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,5 +106,32 @@ struct
106106
; H.section "object: malformed input is rejected"
107107
; H.checkRaises "decodeLoose garbage" (fn () => Git.decodeLoose "not a git object")
108108
; H.checkRaises "parseTree truncated" (fn () => Git.parseTree "100644 big.txt")
109+
110+
(* An object header size is decimal ASCII with no fixed width, so a corrupt
111+
or hostile loose object can carry a size well past 2^31. MLton's default
112+
`int` is 32-bit, so `Int.fromString` on such a numeral raises `Overflow`
113+
-- an *uncontrolled* crash that also diverges from Poly/ML (63-bit int,
114+
which would instead reach the size-mismatch check). The parser must
115+
reject an out-of-range size as a clean `Git` error (never `Overflow`),
116+
identically on both compilers. We assert the specific `Git` constructor,
117+
not merely "some exception", so an `Overflow` crash still fails. *)
118+
; H.section "object: oversized size header is rejected cleanly"
119+
; let
120+
fun raisesGit thunk =
121+
(ignore (thunk ()); false)
122+
handle Git.Git _ => true
123+
| _ => false (* Overflow or anything else -> not a clean reject *)
124+
val loose10 = Zlib.deflateZlib {level = 6} "blob 9999999999\000hello\n"
125+
val loose20 = Zlib.deflateZlib {level = 6} "blob 99999999999999999999\000hello\n"
126+
in
127+
H.check "oversized 10-digit size -> Git (not Overflow)"
128+
(raisesGit (fn () => Git.decodeLoose loose10));
129+
H.check "oversized 20-digit size -> Git (not Overflow)"
130+
(raisesGit (fn () => Git.decodeLoose loose20));
131+
(* a well-formed small object with a size that MATCHES its payload still
132+
decodes cleanly -- the fix must not regress the happy path *)
133+
H.checkEq "well-formed small object still decodes"
134+
(Git.Blob "hi", Git.decodeLoose (Zlib.deflateZlib {level = 6} "blob 2\000hi"))
135+
end
109136
)
110137
end

0 commit comments

Comments
 (0)