diff --git a/NEXT.md b/NEXT.md index 6a16a20..966f2d9 100644 --- a/NEXT.md +++ b/NEXT.md @@ -1321,7 +1321,7 @@ it *would* have written. aborts. Only `SNAP_MAX`/`SNAP_NAMES` is still read rather than tested: sixty-five nested `restart-case`s are a lot of program for a clamp. `escaped[ESCAPE_MAX]` was already covered, because `println.flan` drives a 1100-character string through it on purpose — 1019 bytes out against a worst case of 1021 into 1024. - `scratch[SCRATCH]` never sees more than 20 characters of 64. + The number conversions' buffer never sees more than 20 characters of the 64 the caller supplies. 3. **Valgrind over the headless corpus, done.** `dune build --root . @valgrind` runs forty-nine programs under memcheck, twelve of them again with `--no-bounds-checks`, in 91 seconds including the compiles — on a warm object cache; 162s was measured on a cold one, which is the compiles and not the sweep. Clean. It needs no @@ -2688,26 +2688,26 @@ memcheck sweep (`@valgrind`), whose alarm is looser at 5400s because memcheck is ## Sharp edges -- **Two formatted numbers cannot be held at once.** `flan_i64_to_bytes`, `flan_f64_to_bytes` and `flan_u64_to_bytes` - all write into one `static char scratch[64]` — "rendered text lives here until the next call", flan_rt.c:184 — and - `(string b)` does not copy. So +- **A formatted number does not outlive its frame.** This entry used to say that two of them could not be held at + once, because `flan_i64_to_bytes`, `flan_f64_to_bytes` and `flan_u64_to_bytes` all wrote into one file-static buffer + and `(string b)` does not copy. That part is fixed: the buffer is the caller's now, one frame slot per call site, + allocated by the checker (`check.ml`, `to_bytes`) so that both backends get a function-lifetime location without + either of them reasoning about lifetimes. `test/programs/two-numbers.flan` is the case that used to print `22 22`. + + What is left is the lifetime, and it is the ordinary one a pointer into a frame has: ``` - (let [a (string (i64->bytes 11)) - b (string (i64->bytes 22))] - (print a) (print " ") (println b)) ; => 22 22 + (defn label [n i64] string (string (i64->bytes n))) ; a view of a frame that is gone + (push lines (string (i64->bytes n))) ; every element aliases one slot ``` - `a` is 11 and prints 22. No crash and no diagnostic. This is not new — the `[u8]` already aliased — but a `string` - reads as more value-like and invites exactly this. Format, draw, measure, then format the next one; `digits.flan` - sequences itself strictly for this reason. `rl/draw-text` is safe because the shim's `flan_shim_cstr` copies out of - ptr+len before the call. + Neither is refused today. The first returns a view of storage the return has just released; the second pushes + ptr+len, not the bytes, and a slot reused on the next turn of a loop leaves every element reading as the last + number. Copy the bytes for anything that outlives the expression that made them — which is what the prelude's + `append-i64!` and `append-f64!` do, and the reason that shape exists: they copy into a `(Vec u8)`, so a builder + holds as many rendered numbers as it likes, and `format-f64` answers a `Vec` rather than a view. - **The prelude now has the shape that does not have this problem**, and it is the reason that shape exists. - `append-i64!` and `append-f64!` copy out of the scratch buffer into a `(Vec u8)` before returning, so a builder - holds as many rendered numbers as it likes, and `format-f64` answers a `Vec` rather than a view. The hazard is - unchanged for anyone calling `i64->bytes` directly — nothing was taken away — but a caller assembling a line of - text has a way not to meet it. + `rl/draw-text` is safe for a third reason: the shim's `flan_shim_cstr` copies out of ptr+len before the call. - **Writing through a string literal is undefined, and the two build modes disagree about how.** `(let [s (bytes "Hi")] (set (at s 0) \h))` stores into diff --git a/lib/check.ml b/lib/check.ml index 8411eb2..8f87beb 100644 --- a/lib/check.ml +++ b/lib/check.ml @@ -1186,6 +1186,38 @@ let align_of loc t = mk loc (Types.Int Types.I64) (Tast.Prim (Tast.AlignOf t, [] let addr_of loc (e : Tast.expr) = mk loc (Types.Ptr e.Tast.ty) (Tast.Prim (Tast.AddrOf, [ e ])) +(* ── Where a rendered number's bytes live ────────────────────────────── + + The three number-to-text conversions used to answer a slice into one static + buffer in the runtime, shared by every call in the process, and nothing + copied it: (print a) (print b) over two of them printed the second number + twice. No crash and nothing for a sanitizer to find, because the read was + inside a buffer that was perfectly alive — the wrong bytes, alive. + + The buffer is now the caller's, one frame slot per call site, and it is + allocated here rather than in either backend on purpose: a slot is a + function-lifetime frame location in both of them — an entry-block alloca in + [Emit], a prologue-allocated offset in [X86] — where a backend temporary in + [X86] is bump-allocated and reclaimed at the end of the expression that made + it, which is exactly the lifetime a returned slice must outlive. Doing it + once here also keeps the two backends symmetric by construction: each gains + one pointer argument and no lifetime reasoning of its own. + + 64 bytes is agreed with flan_rt.c's FLAN_NUM_BYTES, which clamps the length + it publishes to it. The zeroing the [Let] does is one 64-byte clear beside + an snprintf. *) +let num_bytes = 64L + +let to_bytes ctx loc pr (x : Tast.expr) = + let bty = Types.Array (num_bytes, Types.Int Types.U8) in + let bslice = Types.Slice (Types.Int Types.U8) in + let s = fresh_slot ctx bty in + mk loc bslice + (Tast.Let + ([ (s, mk loc bty (Tast.Zero bty)) ], + [ mk loc bslice + (Tast.Prim (pr, [ x; addr_of loc (mk loc bty (Tast.Local s)) ])) ])) + (* ── The region requirement, emitted ─────────────────────────────────── spec-memory.md's arena rule, and the whole of what replaced the three refusals a container of owning elements used to meet at its *type*. The @@ -5031,11 +5063,13 @@ and named_call ctx ~want loc name args = Provenance is still what the other direction needs; nothing here depends on having it. - The one sharp edge is not new but is easier to trip over now: the slice - that i64->bytes / f64->bytes / u64->bytes answer is a view into one shared - static buffer in the runtime, overwritten by the next such call. Calling - it a string does not copy it. Use it before formatting the next number; - you cannot hold two at once. *) + The sharp edge left here is one of lifetime and no longer one of sharing: + the slice that i64->bytes / f64->bytes / u64->bytes answer is a view into + a frame slot belonging to *that call site* (see [to_bytes]), so two of + them can be held at once and the text of one survives the making of the + next. What it does not survive is its frame — calling it a string does not + copy it, so storing one in a container or returning it hands back a view + of storage that has been reused. Copy the bytes for that. *) | "string" -> arity loc name 1 args; prim Tast.StrOfBytes Types.String [ byte_slice ctx (List.hd args) ] @@ -5047,12 +5081,14 @@ and named_call ctx ~want loc name args = prim Tast.BytesToI64 (Types.Int Types.I64) [ byte_slice ctx (List.hd args) ] | "f64->bytes" -> arity loc name 1 args; - prim Tast.F64ToBytes (Types.Slice (Types.Int Types.U8)) - [ check ctx ~want:(Types.Float Types.F64) (List.hd args) ] + expect loc ~want + (to_bytes ctx loc Tast.F64ToBytes + (check ctx ~want:(Types.Float Types.F64) (List.hd args))) | "i64->bytes" -> arity loc name 1 args; - prim Tast.I64ToBytes (Types.Slice (Types.Int Types.U8)) - [ check ctx ~want:(Types.Int Types.I64) (List.hd args) ] + expect loc ~want + (to_bytes ctx loc Tast.I64ToBytes + (check ctx ~want:(Types.Int Types.I64) (List.hd args))) | "write-stdout" -> arity loc name 1 args; prim Tast.WriteStdout Types.Unit [ byte_slice ctx (List.hd args) ] @@ -5129,10 +5165,20 @@ and named_call ctx ~want loc name args = else let bslice = Types.Slice (Types.Int Types.U8) in let write x = mk loc Types.Unit (Tast.Prim (Tast.WriteStdout, [ x ])) in - let conv pr x = mk loc bslice (Tast.Prim (pr, [ x ])) in + (* One frame slot per conversion the printer emits, which is what + [to_bytes] is for. The printer writes each number out before making the + next, so a shared buffer would in fact have served it — but the slot is + what the node now carries, and a printer that assembled its own buffer + would be a second answer to the same question. [escape] is the one that + still renders into a static: it is reachable from nowhere but here, and + its 1KB buffer per printed string field is a frame cost with no bug + behind it. Said here so the asymmetry is a decision and not an + oversight. *) + let conv pr x = to_bytes ctx loc pr x in let emitter : Render.emitter = { Render.ebytes = write; - estr = (fun x -> write (conv Tast.EscapeBytes x)); + estr = (fun x -> write (mk loc bslice + (Tast.Prim (Tast.EscapeBytes, [ x ])))); ei64 = (fun x -> write (conv Tast.I64ToBytes x)); eu64 = (fun x -> write (conv Tast.U64ToBytes x)); ef64 = (fun x -> write (conv Tast.F64ToBytes x)) } diff --git a/lib/emit.ml b/lib/emit.ml index 4f72192..47ae003 100644 --- a/lib/emit.ml +++ b/lib/emit.ml @@ -2169,9 +2169,12 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) = | Tast.StrOfBytes, [ x ] -> value f x | Tast.BytesToF64, [ x ] -> shim_in f "@flan_bytes_to_f64" "double" x | Tast.BytesToI64, [ x ] -> shim_in f "@flan_bytes_to_i64" "i64" x - | Tast.F64ToBytes, [ x ] -> shim_out f "@flan_f64_to_bytes" x - | Tast.I64ToBytes, [ x ] -> shim_out f "@flan_i64_to_bytes" x - | Tast.U64ToBytes, [ x ] -> shim_out f "@flan_u64_to_bytes" x + (* The second argument is the caller's buffer — a frame slot the checker gave + this call site, so that two conversions in one expression are two buffers. + See check.ml's [to_bytes]. *) + | Tast.F64ToBytes, [ x; b ] -> shim_out f "@flan_f64_to_bytes" x b + | Tast.I64ToBytes, [ x; b ] -> shim_out f "@flan_i64_to_bytes" x b + | Tast.U64ToBytes, [ x; b ] -> shim_out f "@flan_u64_to_bytes" x b | Tast.EscapeBytes, [ x ] -> shim_in_out f "@flan_escape_bytes" x | Tast.WriteStdout, [ x ] -> let p, n = explode f x in @@ -2263,10 +2266,11 @@ and shim_in f name ret x = ins f "%s = call %s %s(ptr %s, i64 %s)" t ret name p n; t -and shim_out f name (x : Tast.expr) = +and shim_out f name (x : Tast.expr) (buf : Tast.expr) = let v = value f x in + let b = value f buf in let tmp = alloca f (Types.Slice (Types.Int Types.U8)) in - ins f "call void %s(%s %s, ptr %s)" name (ll x.Tast.ty) v tmp; + ins f "call void %s(%s %s, ptr %s, ptr %s)" name (ll x.Tast.ty) v b tmp; load f tmp (Types.Slice (Types.Int Types.U8)) (* Slice in, slice out: [shim_in] returns a scalar and [shim_out] takes one, so @@ -2703,9 +2707,9 @@ declare void @flan_write_stdout(ptr, i64) declare void @flan_exit(i32) declare double @flan_bytes_to_f64(ptr, i64) declare i64 @flan_bytes_to_i64(ptr, i64) -declare void @flan_f64_to_bytes(double, ptr) -declare void @flan_i64_to_bytes(i64, ptr) -declare void @flan_u64_to_bytes(i64, ptr) +declare void @flan_f64_to_bytes(double, ptr, ptr) +declare void @flan_i64_to_bytes(i64, ptr, ptr) +declare void @flan_u64_to_bytes(i64, ptr, ptr) declare void @flan_escape_bytes(ptr, i64, ptr) declare void @flan_handler_push(ptr) declare void @flan_handler_pop(ptr) diff --git a/lib/x86.ml b/lib/x86.ml index a06c10c..0c7249a 100644 --- a/lib/x86.ml +++ b/lib/x86.ml @@ -2668,9 +2668,12 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) dst = (* string and [u8] are the same two words, so both directions are views and not copies — the same non-instruction [emit.ml] emits. *) | (Tast.Bytes | Tast.StrOfBytes), [ a ] -> lower f a dst - | Tast.I64ToBytes, [ a ] -> shim_out f "flan_i64_to_bytes" a dst - | Tast.U64ToBytes, [ a ] -> shim_out f "flan_u64_to_bytes" a dst - | Tast.F64ToBytes, [ a ] -> shim_out f "flan_f64_to_bytes" a dst + (* [b] is the caller's buffer, a frame slot the checker gave this call site. + See check.ml's [to_bytes]: a backend temporary here would be reclaimed at + the end of this expression and the slice outlives it. *) + | Tast.I64ToBytes, [ a; b ] -> shim_out f "flan_i64_to_bytes" a b dst + | Tast.U64ToBytes, [ a; b ] -> shim_out f "flan_u64_to_bytes" a b dst + | Tast.F64ToBytes, [ a; b ] -> shim_out f "flan_f64_to_bytes" a b dst | Tast.EscapeBytes, [ a ] -> let l = eval f a in slice_in_out f "flan_escape_bytes" l dst @@ -2718,17 +2721,23 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) dst = | Tast.Cast target, [ a ] -> cast f a target dst | _ -> unsupported "primitive with %d arguments" (List.length args) -(* [void shim(T, flan_slice *out)] — a scalar in, a slice written through a - hidden out pointer. The three number printers, and nothing else. *) -and shim_out f sym (a : Tast.expr) dst = +(* [void shim(T, uint8_t *buf, flan_slice *out)] — a scalar in, text rendered + into the caller's buffer, and a slice over it written through a hidden out + pointer. The three number printers, and nothing else. + Both operands are evaluated before any argument register is loaded: + evaluating one is arbitrary code and would otherwise overwrite the other. *) +and shim_out f sym (a : Tast.expr) (buf : Tast.expr) dst = let l = eval f a in + let lb = eval f buf in if is_float a.Tast.ty then begin fload f.b ~dst:xmm0 ~mm:(lmem f l ~scratch:r11) ~f64:(f64_of a.Tast.ty); - addr_into f ~reg:rdi dst; + load_loc f ~reg:rdi lb buf.Tast.ty; + addr_into f ~reg:rsi dst; imm_into f ~reg:rax 1L end else begin load_loc f ~reg:rdi l a.Tast.ty; - addr_into f ~reg:rsi dst; + load_loc f ~reg:rsi lb buf.Tast.ty; + addr_into f ~reg:rdx dst; imm_into f ~reg:rax 0L end; call_sym f.b sym diff --git a/runtime/flan_rt.c b/runtime/flan_rt.c index 30e8a7f..f8c7ed4 100644 --- a/runtime/flan_rt.c +++ b/runtime/flan_rt.c @@ -233,21 +233,35 @@ void flan_condition_stacks_reset(void) { /* The conversions are *text*: bytes->f64 parses "12.5", f64->bytes renders it. * calc-me's tokenizer needs the first, the prelude's printers the second. */ -#define SCRATCH 64 -static char scratch[SCRATCH]; /* rendered text lives here until the next call */ +/* Where the rendered text goes, and who owns it. + * + * The buffer belongs to the *caller*: the compiler gives every one of these + * call sites a frame slot of its own and passes its address, so two + * conversions in one expression are two buffers and the text of the first is + * still there while the second is made. It used to be one file-static, shared + * by every call in the process — (print a) (print b) over two conversions + * printed the second number twice, with no crash and nothing for a sanitizer + * to see, because the read was inside a buffer that was perfectly alive. + * + * What this does *not* buy is storage: the slice points into the caller's + * frame, so holding one past the function that made it, or pushing it into a + * container that outlives the frame, is still the caller's problem. Copy the + * bytes for that. The size is agreed with check.ml, which allocates the slot — + * grep FLAN_NUM_BYTES there before changing it here. */ +#define FLAN_NUM_BYTES 64 /* snprintf returns what it *would* have written, not what it did. The three * shims below hand the result back as a slice, so taking that number at face - * value would publish a length past the end of the buffer and every reader of - * that slice would run off it. No format here can reach 64 — %g is at most 13 - * characters and %lld at most 20 — so this clamp cannot fire today; it is here - * because the distance between "cannot fire" and "reads off the end of a - * static buffer" is one format string, and nothing else in the file says so. + * value would publish a length past the end of the caller's buffer and every + * reader of that slice would run off it. No format here can reach 64 — %g is + * at most 13 characters and %lld at most 20 — so this clamp cannot fire today; + * it is here because the distance between "cannot fire" and "reads off the end + * of the frame" is one format string, and nothing else in the file says so. * Found by reading, under a sanitizer sweep that could not have found it: * nothing in the corpus prints a number long enough. */ static int64_t fit(int n) { if (n < 0) return 0; - return n < SCRATCH ? (int64_t)n : (int64_t)(SCRATCH - 1); + return n < FLAN_NUM_BYTES ? (int64_t)n : (int64_t)(FLAN_NUM_BYTES - 1); } /* The length is clamped below *and* above. Above is obvious and was always @@ -282,15 +296,15 @@ int64_t flan_bytes_to_i64(const uint8_t *p, int64_t n) { /* %g so that 3.5 prints as "3.5" and not "3.500000" — calc-me's expected * output is a table of exact strings. */ -void flan_f64_to_bytes(double x, flan_slice *out) { - int n = snprintf(scratch, SCRATCH, "%g", x); - out->ptr = (const uint8_t *)scratch; +void flan_f64_to_bytes(double x, uint8_t *buf, flan_slice *out) { + int n = snprintf((char *)buf, FLAN_NUM_BYTES, "%g", x); + out->ptr = buf; out->len = fit(n); } -void flan_i64_to_bytes(int64_t x, flan_slice *out) { - int n = snprintf(scratch, SCRATCH, "%lld", (long long)x); - out->ptr = (const uint8_t *)scratch; +void flan_i64_to_bytes(int64_t x, uint8_t *buf, flan_slice *out) { + int n = snprintf((char *)buf, FLAN_NUM_BYTES, "%lld", (long long)x); + out->ptr = buf; out->len = fit(n); } @@ -298,9 +312,9 @@ void flan_i64_to_bytes(int64_t x, flan_slice *out) { * not -1, and routing it through the signed printer is the only way println * could disagree with the REPL about a value both can hold. Hence a second * shim rather than a cast at the call site. */ -void flan_u64_to_bytes(uint64_t x, flan_slice *out) { - int n = snprintf(scratch, SCRATCH, "%llu", (unsigned long long)x); - out->ptr = (const uint8_t *)scratch; +void flan_u64_to_bytes(uint64_t x, uint8_t *buf, flan_slice *out) { + int n = snprintf((char *)buf, FLAN_NUM_BYTES, "%llu", (unsigned long long)x); + out->ptr = buf; out->len = fit(n); } diff --git a/test/programs/two-numbers.flan b/test/programs/two-numbers.flan new file mode 100644 index 0000000..5c4a2d9 --- /dev/null +++ b/test/programs/two-numbers.flan @@ -0,0 +1,42 @@ +;;;; Two rendered numbers, held at once. +;;;; +;;;; i64->bytes and its two siblings render into a buffer and answer a slice +;;;; over it. That buffer used to be one file-static in the runtime, shared by +;;;; every call in the process, so the program below printed "22 22": the +;;;; second conversion overwrote the first, and the first slice — still a +;;;; perfectly valid pointer into a perfectly live buffer — was read after it. +;;;; No crash, no diagnostic, and nothing for a sanitizer to catch, because +;;;; every byte read was inside an object that was alive. The wrong bytes. +;;;; +;;;; The buffer is the caller's now, one frame slot per call site, which is why +;;;; the two conversions below do not collide and why the f64 held across an +;;;; i64 conversion — a different shim, and the same buffer before — survives +;;;; it. What the slice still does not outlive is its frame: storing one in a +;;;; container that lives longer, or returning it, hands back a view of storage +;;;; that has been reused. That is copying's job and is said in check.ml. +(defn main [] i32 + ;; Two i64 conversions alive at the same time. + (let [a (string (i64->bytes 11)) + b (string (i64->bytes 22))] + (print a) (print " ") (println b)) ; 11 22 + + ;; Three, and read in the order they were made rather than in reverse, so a + ;; version that rotated among two buffers would still be caught. + (let [a (string (i64->bytes 1)) + b (string (i64->bytes 2)) + c (string (i64->bytes 3))] + (print a) (print b) (println c)) ; 123 + + ;; Across the two shims: the f64's text is made first and read last. + (let [x (string (f64->bytes 2.5)) + n (string (i64->bytes 7))] + (print x) (print " ") (println n)) ; 2.5 7 + + ;; Inside a loop, where the slot is reused per iteration: each turn's text is + ;; read before the next turn writes it, which is the contract a frame slot + ;; gives. Printed on one line so the loop's shape is visible in the output. + (dotimes [i 4] + (let [s (string (i64->bytes (i64 (* i 11))))] + (print s) (print " "))) + (println "") ; 0 11 22 33 + 0) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index b679aa8..fc168eb 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -311,6 +311,17 @@ let () = outputs ~opt:"-O0" "string of bytes, -O0" "programs/string-of-bytes.flan" string_of_bytes_out; + (* Two rendered numbers held at once, which is what one shared buffer in + the runtime made impossible: this printed "22 22" and could not have + been caught by a sanitizer, because every byte read was inside a live + object — the wrong one. -O0 too, since the buffer is now a frame slot + and mem2reg is what decides whether the address escapes. *) + let two_numbers_out = "11 22\n123\n2.5 7\n0 11 22 33 \n" in + outputs "two rendered numbers at once" "programs/two-numbers.flan" + two_numbers_out; + outputs ~opt:"-O0" "two rendered numbers at once, -O0" + "programs/two-numbers.flan" two_numbers_out; + (* The other side of that boundary: bytes the copy cannot represent. A NUL inside the string is where ptr+len and C's "ends at the first NUL" stop describing the same value, so the shim refuses instead of handing C a diff --git a/test/test_sanitize.ml b/test/test_sanitize.ml index 12090cd..38e17fc 100644 --- a/test/test_sanitize.ml +++ b/test/test_sanitize.ml @@ -118,6 +118,12 @@ let corpus = the harness rather than in anything under test. *) "programs/bounds.flan", [ "0" ]; "programs/arena-value.flan", []; + (* Here for what it would catch rather than for what it prints: the three + number conversions render into a frame slot the checker allocates per + call site, and a slot that ended up as a reclaimed temporary instead + would be a stack-use-after-scope — which is exactly what ASan sees and + an output comparison does not. *) + "programs/two-numbers.flan", []; "programs/arena-edn.flan", []; "programs/bytes2.flan", []; "programs/cleanup.flan", [];