From 16717a555695ef666e9d2cd1bacc762a2ab15f4b Mon Sep 17 00:00:00 2001 From: Gabriel Schneider Date: Mon, 21 Sep 2026 23:53:58 -0300 Subject: Fixes from three adversarial reviews, and a destructive one among them The registry sweep could delete a live socket, anywhere on the filesystem. A reviewer reproduced it: a socket that is bound but has not reached listen(2) answers ECONNREFUSED exactly like a dead one -- that window is every server's startup -- and the sweep then followed the entry's symlink and unlinked whatever absolute path it named. It now follows a target only into the directory our own sockets live in and only to a `pardes-9p-*.sock` name, it re-probes immediately before deleting rather than trusting a probe that is by then several syscalls old, and a readlink that exactly filled its buffer is treated as the truncation it is. The test grew a case for an entry whose target is not ours: the entry goes, the file does not. Ctrl-V in raw tty mode was a black hole when the yank register was empty -- neither typed nor forwarded -- so vim's visual block, readline's quoted-insert and every other program's Ctrl-V simply vanished. With nothing to paste the chord belongs to the program again. The lone-ESC flush added earlier was dead code. vaxis already returns Escape for a one-byte 0x1b (`Parser.parseGround` asserts `input.len == 1`), so the carried byte it waited for can never exist; a reviewer showed a 3 ms gap and a 60 ms gap behaving identically. Removed rather than left to imply a guarantee it never provided. A shell whose editor is gone can start one again. Naming a live but unreachable session made `pardes ` exit 1, which let a stale environment variable lock someone out of their own editor; it falls through to an ordinary session, as it did before the variable existed. Also: the macOS ABI check for `pardes_topbar_pane_border_px` had been replaced by a duplicate of the line above it; `--startup` now fails on a leak the way every other measurement in that file does, and stops calling its maximum a p95 below twenty samples; the served README and the skill no longer tell you to write to `data` with `>`, which truncates the whole body before the write lands; `docs/v9fs.md` described the allocate-on-walk design that was rejected; and `test/fs.py` keys nesting off `PARDES_PID`, so its forwarding case stops passing only when the runner happens to be inside a live pardes. fs-test now reaches its one documented pre-existing failure instead of dying early. Suite 778/783 with the two known crashes. Co-Authored-By: Claude Opus 5 (1M context) --- src/9p_io.zig | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 9 deletions(-) (limited to 'src/9p_io.zig') diff --git a/src/9p_io.zig b/src/9p_io.zig index 3d019e0d..7f0134ed 100644 --- a/src/9p_io.zig +++ b/src/9p_io.zig @@ -671,7 +671,7 @@ pub const Listener = struct { log.warn("registry post skipped: cannot create {s}", .{svc}); return; } - sweepRegistry(l.io, svc); + sweepRegistry(l.io, svc, xdg); var entry_buf: [sun_path_len:0]u8 = undefined; const entry = std.fmt.bufPrintSentinel(&entry_buf, "{s}/{s}", .{ svc, name }, 0) catch { log.warn("registry post skipped: name too long: {s}", .{name}); @@ -903,7 +903,21 @@ fn probe(path: [:0]const u8) Probe { /// a definite refusal counts as gone. The socket a stale entry points /// at goes too, but not before a stat agrees it is a socket of ours: a /// plain file answers a connect with the same refusal. -fn sweepRegistry(io: std.Io, svc: [:0]const u8) void { +/// Is this the kind of path this editor is allowed to delete? A registry +/// entry is a symlink we wrote, but its target is just bytes on disk that +/// anyone could have pointed anywhere, so the sweep only ever follows one +/// into the directory our own sockets live in, and only to a name of the +/// shape we give them. Everything else gets its entry removed and its target +/// left strictly alone. +fn ourSocket(target: []const u8, sockets: []const u8) bool { + if (sockets.len == 0 or !std.mem.startsWith(u8, target, sockets)) return false; + if (target.len <= sockets.len or target[sockets.len] != '/') return false; + const base = target[sockets.len + 1 ..]; + if (std.mem.indexOfScalar(u8, base, '/') != null) return false; + return std.mem.startsWith(u8, base, "pardes-9p-") and std.mem.endsWith(u8, base, ".sock"); +} + +fn sweepRegistry(io: std.Io, svc: [:0]const u8, sockets: []const u8) void { if (comptime !supported) return; // The names are staged before anything is unlinked, so the sweep @@ -941,12 +955,22 @@ fn sweepRegistry(io: std.Io, svc: [:0]const u8) void { if (libc.unlink(entry) != 0) continue; reaped += 1; // A relative target would resolve against this editor's working - // directory, which says nothing about what the entry named. + // directory, which says nothing about what the entry named, and a + // readlink that exactly filled the buffer was truncated, so the path + // it produced is some other file's. if (state != .stale or n <= 0 or link_buf[0] != '/') continue; + if (@as(usize, @intCast(n)) >= link_buf.len) continue; var target_buf: [sun_path_len:0]u8 = undefined; const target = std.fmt.bufPrintSentinel(&target_buf, "{s}", .{link_buf[0..@intCast(n)]}, 0) catch continue; + if (!ourSocket(target, sockets)) continue; const t = statNoFollow(target) orelse continue; - if (t.mode & 0o170000 == 0o140000 and t.uid == libc.getuid()) _ = libc.unlink(target); + if (t.mode & 0o170000 != 0o140000 or t.uid != libc.getuid()) continue; + // Ask again, immediately before deleting. The first probe was of the + // ENTRY and is by now several syscalls old; a socket that is bound but + // has not reached listen(2) yet answers ECONNREFUSED exactly like a + // dead one, and that window is every server's startup. + if (probe(target) != .stale) continue; + _ = libc.unlink(target); } if (reaped != 0) log.info("reaped {d} stale registry entries under {s}", .{ reaped, svc }); } @@ -997,14 +1021,23 @@ test "the registry sweep takes the dead entries and leaves everything else" { return error.SkipZigTest; if (libc.mkdir(svc, 0o700) != 0) return error.SkipZigTest; - var paths: [5][sun_path_len:0]u8 = undefined; - const live_sock = try std.fmt.bufPrintSentinel(&paths[0], "{s}/live.sock", .{svc}, 0); - const dead_sock = try std.fmt.bufPrintSentinel(&paths[1], "{s}/dead.sock", .{svc}, 0); + // The sockets live where the real ones do -- beside the registry, not in + // it, and named the way a session names them -- because the sweep only + // follows an entry to a target of exactly that shape and place. + var paths: [6][sun_path_len:0]u8 = undefined; + const pid: u32 = @intCast(libc.getpid()); + const live_sock = try std.fmt.bufPrintSentinel(&paths[0], "{s}/pardes-9p-sweeplive-{d}.sock", .{ base, pid }, 0); + const dead_sock = try std.fmt.bufPrintSentinel(&paths[1], "{s}/pardes-9p-sweepdead-{d}.sock", .{ base, pid }, 0); const live = try std.fmt.bufPrintSentinel(&paths[2], "{s}/live", .{svc}, 0); const dead = try std.fmt.bufPrintSentinel(&paths[3], "{s}/dead", .{svc}, 0); const stranger = try std.fmt.bufPrintSentinel(&paths[4], "{s}/stranger", .{svc}, 0); + // A dead entry pointing at something that is NOT one of our sockets: the + // entry goes, the file it named must not. + const outsider_sock = try std.fmt.bufPrintSentinel(&paths[5], "{s}/sweep-outsider-{d}.sock", .{ base, pid }, 0); + var outsider_buf: [sun_path_len:0]u8 = undefined; + const outsider = try std.fmt.bufPrintSentinel(&outsider_buf, "{s}/outsider", .{svc}, 0); defer { - for ([_][:0]const u8{ live_sock, dead_sock, live, dead, stranger }) |p| _ = libc.unlink(p); + for ([_][:0]const u8{ live_sock, dead_sock, live, dead, stranger, outsider_sock, outsider }) |p| _ = libc.unlink(p); _ = libc.rmdir(svc); } @@ -1021,6 +1054,12 @@ test "the registry sweep takes the dead entries and leaves everything else" { try testing.expectEqual(@as(c_int, 0), libc.symlink(live_sock, live)); try testing.expectEqual(@as(c_int, 0), libc.symlink(dead_sock, dead)); + // Dead too, but its target is not one of our sockets by name, so the + // entry must go and the file it named must survive untouched. + const outside = bindSocket(outsider_sock); + try testing.expect(outside >= 0); + defer _ = libc.close(outside); + try testing.expectEqual(@as(c_int, 0), libc.symlink(outsider_sock, outsider)); // Not a symlink, so not this program's to reason about, even though // connecting to it is refused exactly like the dead socket. try std.Io.Dir.cwd().writeFile(testing.io, .{ .sub_path = stranger, .data = "" }); @@ -1058,7 +1097,7 @@ test "the registry sweep takes the dead entries and leaves everything else" { try testing.expectEqual(Probe.live, probe(live)); try testing.expectEqual(Probe.stale, probe(dead)); - sweepRegistry(testing.io, svc); + sweepRegistry(testing.io, svc, base); // Promptly, and not "eventually": a blocking probe never comes back // at all, so any wall-clock bound at all is the assertion that @@ -1071,6 +1110,8 @@ test "the registry sweep takes the dead entries and leaves everything else" { try testing.expect(statNoFollow(live) != null); try testing.expect(statNoFollow(live_sock) != null); try testing.expect(statNoFollow(stranger) != null); + try testing.expect(statNoFollow(outsider) == null); + try testing.expect(statNoFollow(outsider_sock) != null); try testing.expectEqual(Probe.live, probe(live)); } -- cgit v1.3