From 9585d8c2fe41684d4913b9763917e2e665263725 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Fri, 18 Sep 2026 07:29:09 +0700 Subject: [PATCH] An enum member is checked against the i32 it will be, before anything reasons about it The members of a defenum are i32 at run time, but the reader hands the parser an int64, so a value too large for the type arrived looking ordinary: truncated by the x86 backend, malformed in the LLVM IR, and -- the reason this is a correctness hole and not a nicety -- invisible to the duplicate-value rule sitting right below it. That rule compares int64s, so (defenum E [A 0 B 4294967296]) passed it: the two differ as int64 and are both 0 as i32, and the one check written to catch two names for one number waved through exactly the case it exists for. Each value is now checked where it is resolved, which is before the collision scan runs, so the scan compares the numbers the program will actually have. A value that does not fit is refused rather than quietly made to fit, naming the member, its enum, and the value, with a different sentence for a value that was written and one autoincrement walked into -- nothing in the source wrote 2147483648, so the refusal has to say where it came from before it can say it is wrong. The check is bound with a let rather than inlined into the cons, and that is load-bearing: OCaml leaves :: operand order unspecified and takes the tail first, so an inlined check would run after the recursive Int64.add and let (defenum E [A 9223372036854775807 B]) wrap to min_int and refuse B for a number in no one's source. Bound first, A is refused and the wrap is unreachable. The parser is the only place this needs to happen: Parse.decl is the sole constructor of Ast.Defenum's member values, and Load only re-qualifies the enum's name. Explicit-duplicate aliasing is untouched; that rule is deliberate. --- docs/BUGS-2026-09-18.md | 7 ++--- lib/parse.ml | 59 +++++++++++++++++++++++++++++++++++++++-- test/test_flan.ml | 26 ++++++++++++++++++ 3 files changed, 87 insertions(+), 5 deletions(-) diff --git a/docs/BUGS-2026-09-18.md b/docs/BUGS-2026-09-18.md index 1a3b14c..a143d87 100644 --- a/docs/BUGS-2026-09-18.md +++ b/docs/BUGS-2026-09-18.md @@ -96,9 +96,10 @@ territory; fix or record, the lane's call. - **`reg at` TOCTOU** (`dev.ml:1488`): the stopped-only gate is checked three round trips before the render thunk runs; a `restart` in between lets the thunk chase freed memory with the gate's blessing. -- **defenum values never range-checked to i32** (`parse.ml:994`, `check.ml:1666`): the - collision rule compares i64s, so `[A 0 B 4294967296]` passes and both are 0 at runtime; - autoincrement can overflow silently on --x86. +- ~~**defenum values never range-checked to i32**~~ — FIXED. Every resolved member value, + explicit or autoincremented, is range-checked against i32 in the parser before the collision scan, + so the scan compares the numbers the program will actually have. Out of range is refused + by name (`parse/enum-value-out-of-range`); explicit-duplicate aliasing stays legal. - **NaN sign**: LLVM constant-folds `0.0/0.0` to `nan`, x86 computes `-nan` — a stdout DIFFER on a two-line program. Runtime paths agree (`-nan` both). - **`emit.ml:3369` transient test ignores `new_globals`**: on the `retains=false` path a diff --git a/lib/parse.ml b/lib/parse.ml index a511938..2569ec6 100644 --- a/lib/parse.ml +++ b/lib/parse.ml @@ -987,14 +987,69 @@ let rec decl (f : Form.t) : Ast.decl = | List ({ v = Sym "defenum"; _ } :: args) -> (match args with | [ n; { v = Form.Vec ms; _ } ] -> + let ename = sym n in + (* An enum member is an [i32] at run time. [Shim] lowers the type to + int32_t for C's benefit and [Check] builds every member as a + [Tast.Int (v, I32)] -- but the reader hands this pass an [int64], so + a value the type cannot hold arrives here looking perfectly ordinary. + Left alone it is truncated by the x86 backend and malformed in the + LLVM IR, and worse than either, it defeats the duplicate-value rule + below: that rule compares [int64]s, so in + [(defenum E [A 0 B 4294967296])] the two values differ and the scan + passes, while at run time both members are 0 and a [match] on one is + unreachable through the other. The one rule written to catch two + names for one number waves through the case it exists for. + + So every value is checked against its run-time type here -- refused + if it does not fit, never truncated to fit -- the moment it is + resolved and before any of that reasoning runs, and the scan below + therefore compares the numbers the program will actually have. + + The bounds are the signed 32-bit ones and they are spelled out rather + than borrowed. [Check.in_range] is the function that does exactly + this for every ordinary literal, and it is the right one -- but + [Check] is built above this module and reading its output, so there + is no call this file could make. If one of the two changes, change + the other. *) + let fits m loc ~explicit (v : int64) = + if Int64.compare v (-2147483648L) >= 0 + && Int64.compare v 2147483647L <= 0 + then v + else if explicit then + Loc.failk "parse/enum-value-out-of-range" loc + "the member %s of %s is %Ld, which does not fit i32 — an enum's \ + discriminant is an i32, so its members run from -2147483648 to \ + 2147483647. Give %s a value in that range, or a defconst of a \ + wider type if the number itself is what matters" + m ename v m + else + Loc.failk "parse/enum-value-out-of-range" loc + "the member %s of %s has no value of its own, so it \ + autoincrements to %Ld, which does not fit i32 — an enum's \ + discriminant is an i32, so its members run from -2147483648 to \ + 2147483647. Write %s's value out, or lower the member above it" + m ename v m + in (* Each member becomes its name, its value, whether that value was written, and where the name is. The last two exist only so the - refusal below can be made; neither reaches the AST. *) + refusals here and below can be made; neither reaches the AST. *) let rec members next = function | [] -> [] | { v = Form.Sym m; loc } :: { v = Form.Int k; _ } :: rest -> + (* The [let] is load-bearing rather than tidiness. OCaml leaves the + evaluation order of [::]'s two operands unspecified and in + practice takes the tail first, so an inlined [fits ... k] would + run *after* the recursive call -- and for + [(defenum E [A 9223372036854775807 B])] that recursive call is + [Int64.add max_int 1L], which wraps quietly to min_int and would + have the refusal name B and a number written nowhere in the + source. Binding first refuses A, whose value is the one actually + wrong, and in doing so makes the wrap unreachable: [k] is inside + i32 by the time it is incremented, so the sum cannot overflow. *) + let k = fits m loc ~explicit:true k in (m, k, true, loc) :: members (Int64.add k 1L) rest | { v = Form.Sym m; loc } :: rest -> + let next = fits m loc ~explicit:false next in (m, next, false, loc) :: members (Int64.add next 1L) rest | bad :: _ -> fail bad @@ -1041,7 +1096,7 @@ let rec decl (f : Form.t) : Ast.decl = names, or a value no other member holds" m v other m) indexed; - mk (Ast.Defenum (sym n, List.map (fun (m, v, _, _) -> (m, v)) ms)) + mk (Ast.Defenum (ename, List.map (fun (m, v, _, _) -> (m, v)) ms)) | _ -> fail f "defenum is (defenum Name [member value? ...]). A member with no \ diff --git a/test/test_flan.ml b/test/test_flan.ml index 1d82d55..8af8db3 100644 --- a/test/test_flan.ml +++ b/test/test_flan.ml @@ -589,6 +589,32 @@ let () = "(defenum E [A B 0])" ~needle:"A has no value of its own, so it autoincrements to 0"; + (* A member is an i32 at run time, so a value outside i32 is refused where it + is resolved -- and the first of these is why the check has to happen + *before* the collision scan above rather than after it. 0 and 2^32 are + different int64s and the same i32, so the scan compares them, finds them + unequal, and passes a program in which both members are 0; the range + refusal is what stops it ever reaching that comparison. *) + parse_rejects "an explicit member too large for i32" + "(defenum E [A 0 B 4294967296])" + ~needle:"the member B of E is 4294967296, which does not fit i32"; + parse_rejects "the out-of-range refusal says what the range is" + "(defenum E [A 0 B 4294967296])" + ~needle:"its members run from -2147483648 to 2147483647"; + (* Nothing in the source wrote 2147483648, so the sentence has to say where + it came from before it can say it is wrong. *) + parse_rejects "an autoincrement off the top of i32" + "(defenum E [A 2147483647 B])" + ~needle:"B of E has no value of its own, so it autoincrements to 2147483648"; + (* The int64 end of the same problem. The member refused is [A], the one + whose value is actually wrong: were the range check to run after the + recursive call rather than before it, [Int64.add] would wrap past max_int + and the refusal would name B and the number min_int, which appears nowhere + in the program. This needle is the pin on that ordering. *) + parse_rejects "an explicit member at the top of i64 does not wrap" + "(defenum E [A 9223372036854775807 B])" + ~needle:"the member A of E is 9223372036854775807, which does not fit i32"; + (* Genuinely malformed input still says what a member is, in the grammar the form now has. *) parse_rejects "an enum member that is not a name" "(defenum E [1 A])"