JosephFerano/flan
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Four ways the break loop lied about the program's state

Browse Source 
Found by a concurrency audit that demonstrated three of them against a running
program rather than reasoning about them.

The break state was a flag, not a depth. A C-x C-e thunk may itself error, and
the break loop that catches it nests inside the first - so the inner loop's
resume stored broken = 0 while the outer one was still stopped. Every verb that
could rescue the program then answered "not stopped", status answered "running",
and the outer loop spun forever with no protocol path out. Only kill recovered
it, and Emacs' modeline read live throughout. The audit showed it with ticks
frozen at 0 beside :stopped nil. It is a depth now, capped, and past the cap the
program says so and exits rather than grinding. The condition name is saved and
restored per frame for the same reason.

An idle connection wedged the whole listener. The accept loop is single-threaded
and serves each connection inline on a blocking read, so a client that connected
and sent nothing - an editor killed mid-request - blocked every later request
including the abort that ends a stopped program. Worse, requests the client had
already given up on were served when its socket finally closed, so an abandoned
abort could kill the program minutes later against a state that had moved on.
Two seconds is generous for one line.

chosen_ready was cleared after the resume attempt, so a restart arriving in that
window was answered ok and then erased. It is claimed into a local and cleared
first now, which also keeps strlen off a buffer the listener may be writing.

And aborting was sticky: an abort that passed its check just as the program
resumed stayed armed and would have killed it at the next unhandled error,
minutes later, in unrelated code, giving nobody the chance to choose.
This commit is contained in:
Joseph Ferano 2026-09-11 20:40:57 +07:00
parent 17ef50898d
commit fe1237ccea
1 changed files with 59 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
vendor/agent/flan_agent.c vendored
View File

@ -106,7 +106,16 @@ extern const uint8_t *flan_restart_name(int32_t i, int64_t *len);
/* What the listener thread hands the stopped game thread. One slot, because
* only one thread is ever stopped. */
static _Atomic int broken; /* the game thread is in the loop */
/* A *depth*, not a flag. A thunk this loop runs may itself error, and the
* break loop that catches that one is nested inside this one — so a flag is
* wrong twice over: the inner loop clearing it on resume tells the world the
* program is running while the outer loop is still stopped, and every verb
* then answers "not stopped" while the outer loop spins forever with no
* protocol path out. Only kill recovered it. Counting fixes both. */
static _Atomic int depth;
#define BREAK_MAX 8 /* deep enough to nest, shallow
* enough that a loop of breaks
* stops rather than grinds */
static char chosen[128];
static _Atomic int chosen_ready;
static _Atomic int aborting;
@ -138,6 +147,11 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition,
}
}
fflush(stderr);
/* What the *outer* loop was reporting, restored on the way out: resuming an
* inner break must not leave the outer one describing a condition that has
* already been answered. */
char outer_name[sizeof condition_name];
memcpy(outer_name, condition_name, sizeof outer_name);
{
size_t k = namelen < 0 ? 0 : (size_t)namelen;
if (k >= sizeof condition_name) k = sizeof condition_name - 1;
@ -146,7 +160,16 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition,
}
/* Published last: the name has to be whole before anything advertises that
* there is one to read. */
atomic_store(&broken, 1);
if (atomic_fetch_add(&depth, 1) + 1 > BREAK_MAX) {
/* Printed without going near the hook: whatever is erroring is erroring
* inside the machinery that reports errors. */
fflush(stdout);
fprintf(stderr,
"flan: %d nested break loops — giving up rather than spinning\n",
BREAK_MAX);
fflush(stderr);
exit(134);
}
for (;;) {
flan_agent_poll();
if (atomic_load(&aborting)) {
@ -155,16 +178,29 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition,
exit(134);
}
if (atomic_load(&chosen_ready)) {
int32_t ok =
flan_break_resume((const uint8_t *)chosen, (int64_t)strlen(chosen), xfer);
/* Claimed into a local and the flag cleared *before* the attempt. The
* other order loses a request that was already answered ok: a [restart]
* arriving during the attempt passes its own check, writes a new name
* and sets the flag, and the store below then erases it. Copying also
* keeps strlen off a buffer the listener may be writing. */
char take[sizeof chosen];
memcpy(take, chosen, sizeof take);
atomic_store(&chosen_ready, 0);
int32_t ok =
flan_break_resume((const uint8_t *)take, (int64_t)strlen(take), xfer);
if (ok) {
fprintf(stderr, "flan: resuming at restart %s\n", chosen);
fprintf(stderr, "flan: resuming at restart %s\n", take);
fflush(stderr);
atomic_store(&broken, 0);
memcpy(condition_name, outer_name, sizeof condition_name);
/* Cleared with the resume: an abort that passed its check just as the
* game thread resumed would otherwise stay armed and kill the program
* at the *next* unhandled error, minutes later, in unrelated code,
* giving nobody the chance to choose. */
atomic_store(&aborting, 0);
atomic_fetch_sub(&depth, 1);
return;
}
fprintf(stderr, "flan: no restart named %s is active\n", chosen);
fprintf(stderr, "flan: no restart named %s is active\n", take);
fflush(stderr);
}
nanosleep(&step, NULL);
@ -226,6 +262,18 @@ static void reply(int fd, const char *s) {
/* One connection, one line, one module. Loading here rather than in the game
* thread is the whole reason this thread exists. */
static void serve(int fd) {
/* A deadline on the read. The accept loop is single-threaded and serves each
* connection inline, so a client that connects and then sends nothing - an
* editor killed mid-request, a daemon that crashed between connect and send -
* blocks every later request, including the [abort] that would end a stopped
* program. Worse, the requests the client had already given up on are served
* when its socket finally closes, so an abandoned abort can kill the program
* minutes later against a state that has moved on. Two seconds is generous
* for one line. */
{
struct timeval tv = { 2, 0 };
setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof tv);
}
char line[4096];
size_t n = 0;
for (;;) {
@ -251,7 +299,7 @@ static void serve(int fd) {
* the one question an editor asks without knowing the state already, and
* refusing it would leave nothing to poll. */
if (strcmp(line, "status") == 0) {
if (atomic_load(&broken)) {
if ((atomic_load(&depth) > 0)) {
reply(fd, "stopped ");
reply(fd, condition_name);
reply(fd, "\n");
@ -260,7 +308,7 @@ static void serve(int fd) {
return;
}
if (strcmp(line, "restarts") == 0) {
if (!atomic_load(&broken)) { reply(fd, "err not stopped\n"); return; }
if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; }
int32_t n = flan_restart_count();
for (int32_t i = 0; i < n; i++) {
int64_t len = 0;
@ -274,7 +322,7 @@ static void serve(int fd) {
return;
}
if (strncmp(line, "restart ", 8) == 0) {
if (!atomic_load(&broken)) { reply(fd, "err not stopped\n"); return; }
if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; }
size_t k = strlen(line + 8);
if (k == 0 || k >= sizeof chosen) { reply(fd, "err bad restart name\n"); return; }
/* Checked here, against the stack the stopped thread is holding still,
@ -301,7 +349,7 @@ static void serve(int fd) {
return;
}
if (strcmp(line, "abort") == 0) {
if (!atomic_load(&broken)) { reply(fd, "err not stopped\n"); return; }
if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; }
reply(fd, "ok\n");
atomic_store(&aborting, 1);
return;