diff --git a/test/dune b/test/dune index 4efb846..96d60fa 100644 --- a/test/dune +++ b/test/dune @@ -25,6 +25,9 @@ (glob_files programs/*.flan) ; The reload primitive's host: a C main that dlopens what Build.shared made. (file reload_host.c) + ; A shared object that is not a redefinition module, for the agent's refusal + ; path. Its destructor is what proves the handle was closed rather than lost. + (file noinstall.c) ; test_dev runs the compiler itself: flan dev launches and owns a program. (file %{workspace_root}/bin/main.exe) ; The Emacs client, which test_emacs drives against a real daemon. diff --git a/test/noinstall.c b/test/noinstall.c new file mode 100644 index 0000000..34c6623 --- /dev/null +++ b/test/noinstall.c @@ -0,0 +1,16 @@ +/* A shared object that is not a redefinition module: it loads, and it has no + * flan_reload_install for the agent to find. + * + * The destructor is the observation. The agent used to drop the handle on the + * floor when it refused a module like this one — the mapping stayed, the + * reference count went up, and the one handle that could have brought it down + * was gone. Now the module is closed, because nothing was installed from it + * and so nothing can point into it, and this line appears while the program is + * still running. Waiting for the program to exit would prove nothing: the + * loader runs every destructor at exit whether anything was closed or not. */ +#include + +__attribute__((destructor)) static void unloaded(void) { + printf("unloaded\n"); + fflush(stdout); +} diff --git a/test/programs/agent-queue.flan b/test/programs/agent-queue.flan new file mode 100644 index 0000000..23e25e9 --- /dev/null +++ b/test/programs/agent-queue.flan @@ -0,0 +1,36 @@ +;;;; The job ring between the listener thread and the game thread, and what it +;;;; does when it is full. +;;;; +;;;; The window this needs is "the listener has queued modules the game thread +;;;; has not looked at yet", and that window has to be held open by something +;;;; other than a timer — how long sixty-five connections take on a loaded +;;;; machine is exactly the kind of race a test must not be. So the program +;;;; blocks on stdin: the test fills the ring, checks what the agent said, and +;;;; only then writes the byte that lets the program poll. +(import agent "vendor:agent") + +;;; libc's, declared straight: no aggregate crosses the boundary, so there is +;;; nothing for a shim to do. +(declare stdin-byte [] i32 "getchar") + +(defvar ticks i64) + +(defn tick [] i64 + (set ticks (+ ticks 1)) + ticks) + +(defn main [args [string]] i32 + (if (< (len args) 2) + (do (println "usage: agent-queue ") 2) + (do + (if (< (agent/start (at args 1)) 0) + (do (println "cannot listen") 1) + (do + (println "ready") + (stdin-byte) + ;; How many the ring actually held. Every module queued is installed + ;; here, so this number is the count of slots that survived — which + ;; is the whole claim: a ring that overwrote the slot it was reading + ;; would answer with something else. + (print (agent/poll)) (println "") + 0))))) diff --git a/test/test_agent.ml b/test/test_agent.ml index 557c33b..1e9f4e4 100644 --- a/test/test_agent.ml +++ b/test/test_agent.ml @@ -285,8 +285,108 @@ let () = fail "break loop transcript\n got: %S\n wanted: %S" got want end; + (* ── The job ring, and what a full one does ─────────────────────── *) + + (* Two claims, one program. A module the agent refuses because it carries + no installer is *closed* rather than leaked; and a ring with no room + refuses the module instead of overwriting the slot the game thread is + reading. + + The program blocks on stdin until the test has finished filling the + ring, so neither claim is a race against how fast sixty-five + connections are served. *) + let qsock = tmp "queue.sock" and qout = tmp "queue.out" in + (try Sys.remove qsock with Sys_error _ -> ()); + let qt, ql = Session.create ~file:"programs/agent-queue.flan" () in + let qexe = tmp "queue" in + ignore + (Build.executable ~opts:dev ~csrcs:ql.Load.csrcs ~lflags:ql.Load.lflags + qt.Session.host ~out:qexe); + (* One module, sent many times. dlopen keys on the path, so this is the + same relocation over and over — what is being counted is publishes, and + building sixty-five of them would measure llc instead. *) + let qso = tmp "queue-tick.so" in + let qc = Session.eval qt "(defn tick [] i64 (set ticks (+ ticks 1)) ticks)" in + ignore (Build.shared ~opts:dev ~ir:qc.Session.ir ~out:qso ()); + let noinstall = tmp "noinstall.so" in + let cc = + Printf.sprintf "clang -shared -fPIC -o %s noinstall.c 2>/dev/null" + (Filename.quote noinstall) + in + if Sys.command cc <> 0 then fail "could not build noinstall.so" + else begin + let rfd, wfd = Unix.pipe () in + let qfd = + Unix.openfile qout [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 + in + let qpid = Unix.create_process qexe [| qexe; qsock |] rfd qfd qfd in + Unix.close qfd; + Unix.close rfd; + let qtext () = In_channel.with_open_bin qout In_channel.input_all in + let has needle = + let t = qtext () in + List.exists (String.equal needle) (String.split_on_char '\n' t) + in + if not (await (fun () -> Sys.file_exists qsock && has "ready")) then begin + fail "the queue program never bound its socket"; + (try Unix.kill qpid Sys.sigkill with Unix.Unix_error _ -> ()) + end + else begin + (* A module with no installer. The refusal was always there; what is + new is that the handle is closed, and the destructor saying so + *while the program is still running* is the only way to see it — + at exit the loader would run it either way. *) + let r = send qsock noinstall in + if r <> "err no flan_reload_install\n" then + fail "a module with no installer: %S" r; + if not (await (fun () -> has "unloaded")) then + fail "the refused module was not closed: %S" (qtext ()); + + (* QUEUE slots, then one more. The one more is refused, at the sender, + with a reason — the old code took it, wrote it over slot 0, and + said ok. *) + let queue_size = 64 in + let bad = ref "" in + for _ = 1 to queue_size do + let r = send qsock qso in + if r <> "ok\n" && !bad = "" then bad := r + done; + if !bad <> "" then fail "a module that fitted was refused: %S" !bad; + let full = send qsock qso in + if full <> "err reload queue full; the program is not calling agent/poll\n" + then fail "a full ring did not refuse: %S" full; + + (* Let it poll. Every slot the ring kept is installed here, so the + number is how many survived — 64, not 65 and not some torn count. *) + ignore (Unix.write wfd (Bytes.of_string "\n") 0 1); + Unix.close wfd; + let qstatus = ref (Unix.WEXITED 0) in + let reaped = + await ~ms:5000 (fun () -> + match Unix.waitpid [ Unix.WNOHANG ] qpid with + | 0, _ -> false + | _, s -> qstatus := s; true) + in + if not reaped then begin + (try Unix.kill qpid Sys.sigkill with Unix.Unix_error _ -> ()); + fail "the queue program never finished" + end + else begin + let got = + String.concat "\n" + (List.filter (fun l -> l <> "") + (String.split_on_char '\n' (qtext ()))) + in + if !qstatus <> Unix.WEXITED 0 || got <> "ready\nunloaded\n64" then + fail "job ring\n got: %S\n wanted: %S" got + "ready\nunloaded\n64" + end + end + end; + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) - [ exe; so1; so2; sock; out; bsock; bout; bexe ]; + [ exe; so1; so2; sock; out; bsock; bout; bexe; qexe; qso; qsock; qout; + noinstall ]; if !failures = 0 then print_endline "agent: all tests passed" else begin Printf.printf "\n%d failure(s)\n" !failures; diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index d8cdef8..60f49f2 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -17,8 +17,11 @@ * that is when the swap becomes visible. Nothing else in the program needs to * know the agent exists. * - * Nothing is ever dlclosed: a cell holds an address inside a module's text, - * and unloading it would leave every call site pointing at unmapped memory. + * Nothing that published anything is ever dlclosed: a cell holds an address + * inside a module's text, and unloading it would leave every call site + * pointing at unmapped memory. The two modules that are closed are the ones + * nothing can point into — a transient thunk, which installs no bodies, and a + * module refused before it was queued. * * This is not a protocol. One line per request, the path to load, and a one * line answer. The daemon and its nREPL are a separate program that will speak @@ -57,8 +60,27 @@ int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, uint64_t *len); /* A ring the listener writes and the game thread reads. One producer, one * consumer, so two atomics and no lock — the game thread must never block on - * the loader. Overflow drops the oldest request rather than stalling; a dev - * loop that queues 64 reloads between two frames has a bigger problem. */ + * the loader. + * + * A full ring is *refused*, at the sender, with an error. The previous comment + * here claimed it dropped the oldest request, and nothing did that: [publish] + * never read [tail], so the 65th module overwrote the slot the game thread was + * reading — twenty-four bytes of function pointers, copied field by field with + * no atomic anywhere near them, so the consumer could take half of one job and + * half of another and call it. Silent corruption of the one thing in this file + * that gets *called*. + * + * Of the three honest answers, refusing is the only one that reaches the + * person who asked. Dropping loses a reload the sender was told was ok — the + * same lie in quieter clothes. Blocking stalls the accept loop, which serves + * connections inline, so a program that has stopped polling would also stop + * answering [status] and [abort]: the dev loop would have no way to say + * anything to a program that had stopped listening to it. A refusal is a line + * the daemon can show, and the fix is to call agent/poll. + * + * The check is safe to make separately from the store because there is exactly + * one producer — this thread — so room, once seen, cannot be taken away by + * anyone: the consumer only ever makes more of it. */ #define QUEUE 64 /* [handle] is set only for a module that declared itself transient — one that * ran a thunk and left nothing behind. Everything else is kept mapped forever: @@ -74,12 +96,26 @@ static int listen_fd = -1; static pthread_t listener; static atomic_int started; -static void publish(job j) { +/* Room for one more. Unsigned subtraction, so the answer survives [head] and + * [tail] wrapping; only their difference means anything. */ +static int queue_room(void) { unsigned h = atomic_load_explicit(&head, memory_order_relaxed); + unsigned t = atomic_load_explicit(&tail, memory_order_acquire); + return (h - t) < QUEUE; +} + +/* 0 if the ring is full, having published nothing. Checked here as well as at + * the caller, because a producer that forgot would otherwise reintroduce + * exactly the overwrite this replaced. */ +static int publish(job j) { + unsigned h = atomic_load_explicit(&head, memory_order_relaxed); + unsigned t = atomic_load_explicit(&tail, memory_order_acquire); + if (h - t >= QUEUE) return 0; queue[h % QUEUE] = j; /* Release: the store to the slot must be visible before the index that * advertises it. */ atomic_store_explicit(&head, h + 1, memory_order_release); + return 1; } /* Returns how many modules were installed. Call it between frames. */ @@ -246,6 +282,22 @@ static char condition_name[128]; int32_t flan_agent_poll(void); +/* Every way out of the break loop that is not a resume. [_exit] and not + * [exit], because this runs on the game thread while the listener thread may + * be inside [dlopen] holding the loader lock — and [exit] runs the atexit + * chain and the ELF destructors, which want that same lock. A program asked to + * abort would hang instead of dying, which is the failure mode the break loop + * exists to replace. Nothing here needs an orderly teardown: the streams are + * flushed by hand above every call. + * + * 134 is kept because that is what a trap exits with; see rt_die in + * flan_rt.c. */ +static _Noreturn void die_now(void) { + fflush(stdout); + fflush(stderr); + _exit(134); +} + static void break_loop(const uint8_t *name, int64_t namelen, void *condition, void *xfer) { struct timespec step = { 0, 2000000 }; /* 2ms */ @@ -263,7 +315,7 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, fprintf(stderr, "flan: %d nested break loops - giving up rather than " "spinning\n", BREAK_MAX); fflush(stderr); - exit(134); + die_now(); } { snapshot *s = snap_top(); @@ -302,14 +354,14 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, "flan: %d nested break loops — giving up rather than spinning\n", BREAK_MAX); fflush(stderr); - exit(134); + die_now(); } for (;;) { flan_agent_poll(); if (atomic_load(&aborting)) { fflush(stdout); fprintf(stderr, "flan: aborted at the break loop\n"); - exit(134); + die_now(); } if (atomic_load(&chosen_ready)) { /* Claimed into a local and the flag cleared *before* the attempt. The @@ -585,6 +637,15 @@ static void serve(int fd) { } return; } + /* Before the dlopen, not after it: a module there is no room to queue is + * one there is no point relocating, and refusing here means no handle is + * taken for it at all. Only this thread produces, so room seen now is room + * still there at [publish] below. */ + if (!queue_room()) { + reply(fd, "err reload queue full; the program is not calling " + "agent/poll\n"); + return; + } void *h = dlopen(line, RTLD_NOW | RTLD_LOCAL); if (h == NULL) { reply(fd, "err "); @@ -593,7 +654,19 @@ static void serve(int fd) { return; } install_fn f = (install_fn)(uintptr_t)dlsym(h, "flan_reload_install"); - if (f == NULL) { reply(fd, "err no flan_reload_install\n"); return; } + if (f == NULL) { + /* Closed, and this is not an exception to "nothing is ever dlclosed". + * That rule is about a module something *points into* — a cell holding + * an address in its text. This one published nothing: no installer ran, + * so no cell names it, and it is unreachable the moment this function + * returns. What leaked before was the handle value rather than the + * mapping — dlopen refcounts by path, so re-sending the same bad file + * bumped a count nothing could ever bring down, and the one reference + * that could was dropped on the floor here. */ + dlclose(h); + reply(fd, "err no flan_reload_install\n"); + return; + } /* Optional: only an expression evaluation has one. */ call_fn c = (call_fn)(uintptr_t)dlsym(h, "flan_reload_call"); /* And only one that leaves nothing behind may be unloaded. */ @@ -605,8 +678,12 @@ static void serve(int fd) { * sender as a reset, not as an answer. */ reply(fd, "ok\n"); /* "queued", not "installed": the store happens on the game thread, at a - * time this thread does not get to choose. */ - publish((job){ f, c, transient == NULL ? NULL : h }); + * time this thread does not get to choose. The room was checked before the + * dlopen and only this thread consumes it, so this cannot fail; it is + * asserted rather than assumed because a silent [publish] that did nothing + * is the failure being fixed. */ + if (!publish((job){ f, c, transient == NULL ? NULL : h })) + fprintf(stderr, "flan: reload queue full after it was checked\n"); return; } }