From 5810fa286fd55cf5efb72e6241d83accbc229af5 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 09:27:46 +0700 Subject: [PATCH] A negative slice length read 63 bytes off the end of whatever it pointed at (slice s 2 1) has length 2 - 1 - 2 = -1. flan_bytes_to_i64 and flan_bytes_to_f64 both wrote their clamp as (size_t)n < sizeof buf - 1, and (size_t)(-1) is 18446744073709551615, which is not less than 511 -- so k took the cap and the memcpy copied 63 or 511 bytes out of a five-byte string constant. ASan calls it a global-buffer-overflow in flan_bytes_to_i64; the regression case is in test_sanitize. Every other (ptr, len) entry point in the runtime already guarded the negative case -- flan_write_stdout tests n > 0, flan_escape_bytes and flan_dev_emit both fold a negative length to zero -- so this was two exceptions rather than a missing convention. A checked build traps on the reversed slice before reaching either, which is why it took an --no-bounds-checks run to show. Also clamps the three snprintf shims that publish scratch as a slice. snprintf returns what it would have written, not what it did, so a format that overran the 64-byte buffer would hand out a length past its end. No format here can: %g is 13 characters and %lld is 20. Found by reading, and the sweep could not have found it -- nothing in forty programs prints a number that long. --- runtime/flan_rt.c | 38 +++++++++++++++++++++++++++++++++----- test/test_sanitize.ml | 35 ++++++++++++++++++++++++++--------- 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/runtime/flan_rt.c b/runtime/flan_rt.c index 0a01a08..c649104 100644 --- a/runtime/flan_rt.c +++ b/runtime/flan_rt.c @@ -184,9 +184,37 @@ void flan_exit(int32_t status) { #define SCRATCH 64 static char scratch[SCRATCH]; /* rendered text lives here until the next call */ +/* 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. + * 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); +} + +/* The length is clamped below *and* above. Above is obvious and was always + * here. Below was not, and it was the real one: a slice's length is a signed + * 64-bit count, (slice s 2 1) computes 2 - 1 - 2 = -1, and `(size_t)n` on a + * negative n is 18446744073709551615, which is not less than 511, so k became + * 511 and the memcpy read 511 bytes from wherever the slice pointed. A checked + * build traps on the reversed slice before it gets here; an unchecked one does + * not, and every other (ptr, len) entry point in this file — flan_write_stdout, + * flan_escape_bytes, flan_dev_emit — already guards the negative case. These + * two were the exceptions. */ +static size_t clamp_len(int64_t n, size_t cap) { + if (n <= 0) return 0; + return (uint64_t)n < (uint64_t)cap ? (size_t)n : cap; +} + double flan_bytes_to_f64(const uint8_t *p, int64_t n) { char buf[512]; - size_t k = (size_t)n < sizeof buf - 1 ? (size_t)n : sizeof buf - 1; + size_t k = clamp_len(n, sizeof buf - 1); memcpy(buf, p, k); buf[k] = '\0'; return strtod(buf, NULL); @@ -194,7 +222,7 @@ double flan_bytes_to_f64(const uint8_t *p, int64_t n) { int64_t flan_bytes_to_i64(const uint8_t *p, int64_t n) { char buf[64]; - size_t k = (size_t)n < sizeof buf - 1 ? (size_t)n : sizeof buf - 1; + size_t k = clamp_len(n, sizeof buf - 1); memcpy(buf, p, k); buf[k] = '\0'; return (int64_t)strtoll(buf, NULL, 10); @@ -205,13 +233,13 @@ int64_t flan_bytes_to_i64(const uint8_t *p, int64_t n) { void flan_f64_to_bytes(double x, flan_slice *out) { int n = snprintf(scratch, SCRATCH, "%g", x); out->ptr = (const uint8_t *)scratch; - out->len = n < 0 ? 0 : (int64_t)n; + 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; - out->len = n < 0 ? 0 : (int64_t)n; + out->len = fit(n); } /* u64 is not i64 with a flag: 0xFFFFFFFFFFFFFFFF is 18446744073709551615 and @@ -221,7 +249,7 @@ void flan_i64_to_bytes(int64_t x, flan_slice *out) { 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; - out->len = n < 0 ? 0 : (int64_t)n; + out->len = fit(n); } /* A string *inside* a printed structure, quoted and escaped, so that the run diff --git a/test/test_sanitize.ml b/test/test_sanitize.ml index 92b9099..770e3e6 100644 --- a/test/test_sanitize.ml +++ b/test/test_sanitize.ml @@ -166,19 +166,14 @@ let sweep ~checks label = anything. Both are written here rather than kept in test/programs because neither is a program anybody should build: one reads off the end of an array and the other shifts an i32 by 32. *) -let control ~expect_report name src = +let control ~expect_report ?(args = []) ~why name src = let path = Filename.concat scratch (name ^ ".flan") in Out_channel.with_open_bin path (fun ch -> Out_channel.output_string ch src); let exe = compile ~sanitize:true ~checks:false path in - let _, text = run exe [] in + let _, text = run exe args in (match expect_report, reported text with - | true, false -> - fail "control %s: nothing reported, so this sweep is not instrumenting \ - Flan code at all — check that Emit still writes the \ - sanitize_address attribute group" name - | false, true -> - fail "control %s: reported, which contradicts what this file says UBSan \ - reaches. Good news; rewrite the comment." name + | true, false -> fail "control %s: nothing reported. %s\n%s" name why text + | false, true -> fail "control %s: reported. %s\n%s" name why text | _ -> ()); (try Sys.remove exe with Sys_error _ -> ()); (try Sys.remove path with Sys_error _ -> ()) @@ -245,6 +240,10 @@ let () = enough past the end lands beyond the redzone and is not seen — which is itself worth knowing about what this tool can do. *) control ~expect_report:true "flan-san-ctl-oob" + ~why:"This sweep is not instrumenting Flan code at all — check that \ + Emit still puts every define in the sanitize_address attribute \ + group, without which -fsanitize=address covers the runtime's C \ + and nothing else." "(defvar arr [4 i32])\n\ (defn main [] i32\n\ \ (set (at arr 0) 1)\n\ @@ -254,9 +253,27 @@ let () = and invisible to UBSan here because nothing emitted a check for it. If this ever starts reporting, the note above is stale. *) control ~expect_report:false "flan-san-ctl-shift" + ~why:"UBSan has started seeing Flan code, which contradicts what this \ + file and Build.opts both say it reaches. Good news; rewrite them." "(defn main [] i32\n\ \ (let [x 1] (let [n 32] (print (<< x n)) (println \"\")))\n\ \ 0)\n"; + (* A regression case, and the one defect this exercise found: a slice's + length is signed, (slice s 2 1) is -1, and flan_bytes_to_i64 cast that + to size_t before comparing it against its buffer — so the memcpy copied + 63 bytes out of whatever the slice pointed at. Every other (ptr, len) + entry point in the runtime already guarded the negative case; these two + were the exceptions. A checked build traps on the reversed slice long + before this, which is why it needs an unchecked build to show. *) + control ~expect_report:false ~args:[ "2" ] "flan-san-ctl-negslice" + ~why:"flan_bytes_to_i64 or flan_bytes_to_f64 is reading off the end of \ + a negative-length slice again — see clamp_len in flan_rt.c." + "(defn main [args [string]] i32\n\ + \ (let [s (bytes \"42\")\n\ + \ n (i32 (bytes->i64 (bytes (at args 1))))]\n\ + \ (print (bytes->i64 (slice s n 1)))\n\ + \ (println \"\"))\n\ + \ 0)\n"; sweep ~checks:true "checked"; unchecked_controls (); if !failures = 0 then print_endline "sanitizer sweep: clean"