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

@@ -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

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"