diff --git a/docs/BUILT.md b/docs/BUILT.md index 99f009c..ddf6446 100644 --- a/docs/BUILT.md +++ b/docs/BUILT.md @@ -2561,6 +2561,11 @@ nothing marked it — which is precisely what a static rule cannot see. exists for is not implemented: a slice is ptr+len and has nowhere to carry the Vec's identity or its generation. Said plainly here rather than implied by the word's presence in the header. +It is not "not yet", either, and the runtime's own comment now says so. A reader for that word is a third word on every +slice in the language — a layout `spec-memory.md` fixes — so implementing the trap is a spec amendment and an ABI +change, not a runtime patch. The two live options are that amendment, or dropping the word from the header and from the +spec together; neither is a cleanup, and until one is taken the word is carried and trusted by nothing. + ### What this leaves for steps 5 to 7 `(Map K V)` is built — see below. What is left: `drop` and with it the transitive move-only rule, recursive teardown, diff --git a/lib/build.ml b/lib/build.ml index 319a396..4a21ccb 100644 --- a/lib/build.ml +++ b/lib/build.ml @@ -645,7 +645,17 @@ let clang_stamp = lazy (stamp_of clang) the source text, the compiler and the flags are all unchanged. The key has to carry [opt] and [target]: the acceptance table builds the same programs at -O0 and -O2, and an -O2 object must not serve an -O0 build. *) -let compile_c ~opts ?tflags ~src ~name () = +(* Warnings for the translation units this project *owns*, which is the runtime + and the dev half of it. They are not on for a package's C or for the + generated shim: a package's sources are someone else's code, and a warning + nobody in this repository can fix is noise on every build that imports it. + The runtime is the opposite case — an unused result, a sign compare or a + conversion that narrows is a bug report here, and the file had none of this + coverage before. [-Werror] is deliberately absent: a clang upgrade must not + stop a user's build over a new diagnostic in code they did not write. *) +let runtime_warnings = [ "-Wall"; "-Wextra" ] + +let compile_c ~opts ?tflags ?(warn = []) ~src ~name () = (* The whole flag list, not just the triple: on wasm32 the sysroot and the resource directory decide which headers and which builtins an object was built against, so repointing either must not serve a stale .o. *) @@ -657,7 +667,8 @@ let compile_c ~opts ?tflags ~src ~name () = (String.concat "\000" [ name; src; stamp_of cc; opts.opt; String.concat " " (cflags opts); - String.concat " " tflags ])) + String.concat " " tflags; + String.concat " " warn ])) in let obj = Filename.concat (cachedir ()) (key ^ ".o") in if not (Sys.file_exists obj) then begin @@ -671,6 +682,7 @@ let compile_c ~opts ?tflags ~src ~name () = String.concat " " ([ Filename.quote cc; opts.opt ] @ cflags opts + @ warn @ [ "-c" ] @ tflags @ [ Filename.quote c; "-o"; Filename.quote tmp ]) in @@ -796,13 +808,13 @@ let executable ?(opts = default) ?(csrcs = []) ?(lflags = []) ?(pnames = []) rather than as a missing flag. The table is BSS, so this costs address space and not binary size, and [-rdynamic] and the cells are still what [--dev] means. *) - let cc src name = compile_c ~opts ~tflags ~src ~name () in + let cc ?(warn = []) src name = compile_c ~opts ~tflags ~warn ~src ~name () in (* The runtime's own C wants -g too, or a backtrace that passes through flan_error lands in a frame with no line. The flag is part of the object cache key via [compile_c]'s [opt]/[tflags] digest — see [cflags]. *) let objs = - cc Runtime_src.source "flan_rt.c" - :: [ cc Runtime_src.dev_source "flan_dev.c" ] + cc ~warn:runtime_warnings Runtime_src.source "flan_rt.c" + :: [ cc ~warn:runtime_warnings Runtime_src.dev_source "flan_dev.c" ] (* wasi-libc's entry point, which is not [main]. See [wasm_main_source]. Not the browser's: emscripten's start code calls [main] under that name, so the .ll's @main is already the entry point and the shim would be a @@ -1053,10 +1065,10 @@ let macro_module ?(opts = default) ?(csrcs = []) ?(lflags = []) ~macros link time, not at codegen, which is the same trap [shared] meets and answers with -relocation-model=pic. *) let tflags = target_flags opts @ [ "-fPIC" ] in - let cc src name = compile_c ~opts ~tflags ~src ~name () in + let cc ?(warn = []) src name = compile_c ~opts ~tflags ~warn ~src ~name () in let objs = - cc Runtime_src.source "flan_rt.c" - :: [ cc Runtime_src.dev_source "flan_dev.c" ] + cc ~warn:runtime_warnings Runtime_src.source "flan_rt.c" + :: [ cc ~warn:runtime_warnings Runtime_src.dev_source "flan_dev.c" ] @ (match p.Tast.cshim with | [] -> [] | parts -> diff --git a/lib/check.ml b/lib/check.ml index 13c9203..3b140f2 100644 --- a/lib/check.ml +++ b/lib/check.ml @@ -4736,7 +4736,8 @@ and named_call ctx ~want loc name args = (* Fills the Vec the line above sized. A file that grew since the measurement is truncated to the buffer; one that shrank leaves a shorter Vec. Both are successful reads of what was there. *) - try_ (rt loc (Types.Int Types.I8) "flan_slurp_into" [ vv (); psv () ]) ] + try_ (rt loc (Types.Int Types.I8) "flan_slurp_into" + [ vv (); psv (); size_of loc u8; here loc ]) ] in expect loc ~want (mk loc vt diff --git a/lib/emit.ml b/lib/emit.ml index c9e771a..5ef246b 100644 --- a/lib/emit.ml +++ b/lib/emit.ml @@ -2810,7 +2810,7 @@ declare i64 @flan_hash_combine(i64, i64) declare i8 @flan_file_size(ptr, i64, ptr) declare i8 @flan_file_write(ptr, i64, ptr, i64) declare i64 @flan_file_fail_reason() -declare i8 @flan_slurp_into(ptr, ptr, i64) +declare i8 @flan_slurp_into(ptr, ptr, i64, i64, ptr, i64) |} (* C's main, adapting to whichever of the four shapes Flan's main has: argv and diff --git a/lib/shim.ml b/lib/shim.ml index c3958cf..4e65881 100644 --- a/lib/shim.ml +++ b/lib/shim.ml @@ -306,11 +306,21 @@ let header = what keeps that allocation-free in the overwhelmingly common case, and it is 256 because that covers a title, a path and a line of text without making every foreign call carry a page of stack. The only truncation left is when - malloc itself fails, where the alternative is handing C a null pointer. *) + malloc itself fails, where the alternative is handing C a null pointer. + + An embedded NUL is refused rather than copied, which is the policy + flan_path_cstr has always had for a path and which a title, a name or a + query needs for the same reason: C reads to the first NUL, so what crosses + would be a prefix of the string the program passed and the function would + act on a value nobody wrote. The refusal is the runtime's — the shim has no + condition channel — and it names the declare-c it came from. *) let cstr_helpers = - "static char *flan_shim_cstr(const char *p, int64_t n, char *buf, size_t cap) {\n\ + "_Noreturn void flan_shim_nul_fail(const char *site);\n\n\ + static char *flan_shim_cstr(const char *p, int64_t n, char *buf, size_t cap,\n\ + \ const char *site) {\n\ \ size_t len = n <= 0 ? 0 : (size_t)n;\n\ \ char *d = buf;\n\ + \ if (len != 0 && memchr(p, '\\0', len) != NULL) flan_shim_nul_fail(site);\n\ \ if (len + 1 > cap) {\n\ \ d = (char *)malloc(len + 1);\n\ \ if (d == NULL) { d = buf; len = cap - 1; } /* out of memory: truncate */\n\ @@ -386,9 +396,11 @@ let c_for (s : shim) = | Pstr -> let a = arg_name i in Printf.bprintf b " char %s_b[%d];\n" a cstr_cap; + (* The Flan name travels with the copy so that a refusal names the + call the way every other runtime trap names its site. *) Printf.bprintf b - " char *%s = flan_shim_cstr(%s_p, %s_n, %s_b, sizeof %s_b);\n" a a a - a a + " char *%s = flan_shim_cstr(%s_p, %s_n, %s_b, sizeof %s_b, %S);\n" + a a a a a s.sflan | _ -> ()) s.sargs; let call_args = diff --git a/runtime/flan_rt.c b/runtime/flan_rt.c index ba767a3..5f0ffac 100644 --- a/runtime/flan_rt.c +++ b/runtime/flan_rt.c @@ -157,9 +157,28 @@ void flan_rt_init(int32_t argc, char **argv) { setvbuf(stdout, NULL, _IOLBF, 0); } +/* Defined below with the rest of the non-local exits, and forward-declared + * here because the argument vector is built long before them. */ +static _Noreturn void rt_die(void); + +/* The one malloc in this file that is not an allocator's, because the argument + * vector belongs to the process rather than to any region a Flan program named. + * A failure here cannot be a condition: [argv] has no allocation site for the + * compiler to wrap in a restart, and answering with a shorter vector — or with + * a null pointer and a length — is the silently-wrong answer every other entry + * point in this file refuses to give. It cannot fire in practice: this is a + * handful of words asked for before the program has allocated anything. */ void flan_argv(flan_slice *out) { if (rt_args == NULL && rt_argc > 0) { rt_args = (flan_slice *)malloc(sizeof(flan_slice) * (size_t)rt_argc); + if (rt_args == NULL) { + fflush(stdout); + fprintf(stderr, + "flan: out of memory building the argument vector for %d " + "arguments\n", + rt_argc); + rt_die(); + } for (int i = 0; i < rt_argc; i++) { rt_args[i].ptr = (const uint8_t *)rt_argv[i]; rt_args[i].len = (int64_t)strlen(rt_argv[i]); @@ -1252,7 +1271,16 @@ _Noreturn void flan_region_only_fail(const uint8_t *loc, int64_t loclen) { * * ptr len cap allocator the release layout spec-memory.md fixes * gen bumped on every reallocation — the stale-slice - * word. It has no reader yet; see docs/BUILT.md. + * word spec-memory.md asks for. It has no reader + * and cannot have one as things stand, which is + * the part "not yet" used to hide: a slice is + * ptr+len, so it carries neither the Vec it came + * from nor the generation it was taken at, and + * the check has nothing to compare. Giving it a + * reader is a third word on every slice in the + * language, not a change to this file. Nothing + * here or anywhere else reads it; do not write + * code that trusts it. See docs/BUILT.md. * epoch the allocator's epoch when this Vec last * touched it. Any operation on a container whose * recorded epoch has moved traps. @@ -2720,6 +2748,26 @@ int64_t flan_file_fail_reason(void) { return flan_file_fail; } * silently opened, which is the failure this exists to avoid. */ #define FLAN_PATH_MAX 4096 +/* The same policy at the other boundary, for the generated FFI shim. + * + * flan_path_cstr refuses an embedded NUL because the file opened would not be + * the file named; a string handed to any other C function is no different — + * the callee reads to the first NUL, so what crosses is a prefix of the value + * the program passed, and every C API that takes a name, a title or a query + * would act on the wrong one. The shim cannot signal: a foreign call has no + * allocation site for the compiler to wrap and no transfer channel of its own, + * so this traps naming the declare-c that was called, the way an out-of-bounds + * index traps naming its site. See lib/shim.ml, which emits the call. */ +_Noreturn void flan_shim_nul_fail(const char *site) { + fflush(stdout); + fprintf(stderr, + "%s: a string passed to C contains a NUL byte — C reads to the " + "first one, so the value this function would act on is a prefix of " + "the one passed. Remove the NUL before the call.\n", + site); + rt_die(); +} + static int flan_path_cstr(const uint8_t *p, int64_t n, char *out) { if (n < 0 || n >= FLAN_PATH_MAX) return 0; if (n > 0) memcpy(out, p, (size_t)n); @@ -2827,9 +2875,24 @@ int8_t flan_file_write(const uint8_t *path, int64_t n, const void *src, * allocate is StorageExhausted with retry, and a failure to read is FileError * with retry and use-value. Two failures, two conditions, neither swallowing * the other. */ -int8_t flan_slurp_into(flan_vec *v, const uint8_t *path, int64_t n) { - int64_t got = 0; - if (!flan_file_read(path, n, v->ptr, v->cap, &got)) return 0; - v->len = got; +int8_t flan_slurp_into(flan_vec *v, const uint8_t *path, int64_t n, + int64_t size, const uint8_t *loc, int64_t loclen) { + int64_t got = 0, room; + /* The same check every other operation on a container runs, and skipped here + * until now: the Vec was sized on this turn, but a handler between the + * sizing and the read can have released the region it lives in, and this is + * the one entry point that would have written into it anyway. */ + flan_vec_check(v, loc, loclen); + /* A capacity is a count of elements and a read is a count of bytes. They + * were the same number while slurp answered only (Vec u8) — the checker + * still pins it to that — and the conflation was a byte count one element + * size away from being wrong. The product is the block flan_vec_init already + * allocated, so it is representable by construction; the guard is here + * because "by construction" is an argument and not a check. */ + if (!flan_mul_bytes(v->cap, size, &room)) return 0; + if (!flan_file_read(path, n, v->ptr, room, &got)) return 0; + /* Whole elements only: a file that ends mid-element leaves the partial one + * out rather than publishing a length that covers bytes nobody wrote. */ + v->len = size > 0 ? got / size : 0; return 1; } diff --git a/test/programs/shim-nul.flan b/test/programs/shim-nul.flan new file mode 100644 index 0000000..64e3d31 --- /dev/null +++ b/test/programs/shim-nul.flan @@ -0,0 +1,28 @@ +;;;; A string with a NUL in it, handed to C. +;;;; +;;;; A Flan string is ptr+len and a C string ends at its first NUL, so the two +;;;; disagree about what the value *is* the moment one of those bytes is in the +;;;; middle. The generated shim copies and terminates — string-of-bytes.flan +;;;; pins that — and a copy of these bytes is a C string of length 1 where the +;;;; program passed 5. The function would then act on a value nobody wrote. +;;;; +;;;; flan_path_cstr has always refused this for a path, on the grounds that the +;;;; file opened would not be the file named. Nothing about a path is special: +;;;; the same reasoning covers a window title, a shader name and a query, so +;;;; the shim refuses it too, naming the declare-c that was called. It is a +;;;; trap rather than a condition because a foreign call has no allocation site +;;;; for the compiler to guard and no transfer channel of its own. +(declare-c c-puts [s string] i32 "puts") + +(defn main [] i32 + (let [v (vec-new u8)] + (push v 104) ; h + (push v 105) ; i + (push v 0) + (push v 104) ; h + (push v 105) ; i + (println "before") + (c-puts (string (as-slice v))) + (println "unreachable") + (free v)) + 0) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 61dffa2..149907d 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -310,6 +310,26 @@ let () = outputs "string of bytes" "programs/string-of-bytes.flan" string_of_bytes_out; outputs ~opt:"-O0" "string of bytes, -O0" "programs/string-of-bytes.flan" string_of_bytes_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 + prefix — the policy flan_path_cstr has always had for a path. The + refusal names the declare-c, which is the only name the program's author + wrote. *) + let exe = compile "programs/shim-nul.flan" in + let code, text = run exe None in + if code <> 134 || not (contains text "before") + || not (contains text "c-puts: a string passed to C contains a NUL byte") + || contains text "unreachable" + then begin + incr failures; + Printf.printf + "FAIL a string with a NUL in it is refused at the C boundary\n\ + \ got: %S (exit %d)\n wanted: exit 134, naming the call\n" + text code + end; + (try Sys.remove exe with Sys_error _ -> ()); (* handler-bind and signal, spec-conditions.md §1 and §2: signal returns Unit and carries on, an unhandled one is a no-op, a nested frame does not displace the one outside it, and the stack is restored after. *) @@ -2238,10 +2258,12 @@ ERR@7 unexpected token: not the kind the caller was reading The buffer is sized here and not per call site, because a generator has no call site to look at: 256 on the stack, the heap past that, and the copy is freed after the call rather than before the return value is - computed. *) + computed. The Flan name travels with the copy so that the refusal a NUL + in the bytes raises can name the call — see programs/shim-nul.flan. *) shim_case "declare-c: a string is copied, NUL-terminated and freed" "(declare-c open-it [path string] bool \"OpenIt\")" - [ "char a0_b[256];"; "flan_shim_cstr(a0_p, a0_n, a0_b, sizeof a0_b)"; + [ "char a0_b[256];"; + "flan_shim_cstr(a0_p, a0_n, a0_b, sizeof a0_b, \"open-it\")"; "bool r = OpenIt(a0);"; "flan_shim_cstr_free(a0, a0_b);"; " return r;\n" ]; shim_case "declare-c: two strings get two buffers"