Merge pull request #2 from midasdf/fix/adversarial-review-server-safety

Fix close_tab corruption, active_panes routing, and blocking writes

Author: midasdf

build.zig.zon

@@ -1,6 +1,6 @@
 .{
     .name = .zplit,
-    .version = "0.1.0",
+    .version = "0.1.1",
     .fingerprint = 0xbce5f8607c44d1ad,
     .minimum_zig_version = "0.15.0",
     .paths = .{ "build.zig", "build.zig.zon", "src" },

src/main.zig

@@ -477,7 +477,7 @@ pub fn main() !void {
         return;
     }
     if (std.mem.eql(u8, subcmd, "--version") or std.mem.eql(u8, subcmd, "-v")) {
-        std.debug.print("zplit v0.1.0\n", .{});
+        std.debug.print("zplit v0.1.1\n", .{});
         return;
     }
 

src/pane.zig

@@ -405,7 +405,7 @@ fn replaceNodeInParent(
     }
 }
 
-fn firstLeafId(node: *const LayoutNode) PaneId {
+pub fn firstLeafId(node: *const LayoutNode) PaneId {
     return switch (node.*) {
         .leaf => |l| l.id,
         .split => |s| firstLeafId(s.first),

src/server.zig

@@ -134,6 +134,7 @@ pub const ClientState = struct {
     mode: mode_mod.Mode = .normal,
     recv_buf: [RECV_BUF_SIZE]u8 = undefined,
     recv_len: usize = 0,
+    write_failed: bool = false,
     allocator: std.mem.Allocator,
 
     pub fn init(allocator: std.mem.Allocator, id: u16, fd: posix.fd_t) !*ClientState {
@@ -347,7 +348,7 @@ pub const Server = struct {
 
                 if (tag == TAG_LISTEN) {
                     // Accept a new client connection
-                    const new_fd = posix.accept(self.listen_fd, null, null, posix.SOCK.CLOEXEC) catch continue;
+                    const new_fd = posix.accept(self.listen_fd, null, null, posix.SOCK.CLOEXEC | posix.SOCK.NONBLOCK) catch continue;
                     if (self.clients.count() >= MAX_CLIENTS) {
                         posix.close(new_fd);
                         continue;
@@ -448,6 +449,18 @@ pub const Server = struct {
                 cs.rows = hp.rows;
                 cs.screen.resize(hp.cols, hp.rows) catch {};
                 cs.screen.invalidate();
+                // Initialize active_panes for all existing tabs and pick a valid active_tab
+                var first_open_tab: ?u8 = null;
+                for (self.tab_manager.tabs, 0..) |maybe_tab, i| {
+                    if (maybe_tab) |t| {
+                        const tab_idx: u8 = @intCast(i);
+                        cs.active_panes[tab_idx] = pane_mod.firstLeafId(t.pane_tree.root);
+                        if (first_open_tab == null) first_open_tab = tab_idx;
+                    }
+                }
+                if (first_open_tab) |tab_idx| {
+                    cs.active_tab = tab_idx;
+                }
                 if (self.active_client == null) {
                     self.active_client = client_id;
                 }
@@ -557,15 +570,23 @@ pub const Server = struct {
                 const new_tab_idx = self.tab_manager.createTab(new_pane_id) catch return;
                 self.spawnPaneState(new_pane_id) catch return;
                 cs.active_tab = new_tab_idx;
-                cs.active_panes[new_tab_idx] = new_pane_id;
+                // Propagate new tab's focused pane to ALL clients
+                var it = self.clients.valueIterator();
+                while (it.next()) |other_cs| {
+                    other_cs.*.active_panes[new_tab_idx] = new_pane_id;
+                }
                 self.invalidateAllClients();
                 self.composeAll();
             },
             .close_tab => {
-                self.destroyAllPanesInTab(cs.active_tab);
-                const nearest = self.tab_manager.closeTab(cs.active_tab) orelse return;
-                // Update ALL clients viewing the closed tab
+                // Reject closing the last tab before destroying any state
+                if (self.tab_manager.tabCount() <= 1) return;
                 const closed_tab = cs.active_tab;
+                // Clean up per-pane client state (scroll_offsets) before destroying
+                self.cleanupClientStateForTab(closed_tab);
+                self.destroyAllPanesInTab(closed_tab);
+                const nearest = self.tab_manager.closeTab(closed_tab) orelse return;
+                // Update ALL clients viewing the closed tab
                 var it = self.clients.valueIterator();
                 while (it.next()) |other_cs| {
                     if (other_cs.*.active_tab == closed_tab) {
@@ -822,6 +843,8 @@ pub const Server = struct {
         while (it.next()) |cs| {
             self.composeForClient(cs.*);
         }
+        // Disconnect clients that failed to write (corrupted protocol stream)
+        self.sweepFailedClients();
     }
 
     // ── resizePtysForClient ──────────────────────────────────────────────
@@ -989,6 +1012,11 @@ pub const Server = struct {
 
         // Swap buffers
         cs.screen.swapBuffers();
+
+        // Disconnect clients that failed to write during this render
+        if (cs.write_failed) {
+            self.sweepFailedClients();
+        }
     }
 
     // ── sendDirtyRegionsTo ───────────────────────────────────────────────
@@ -1185,8 +1213,17 @@ pub const Server = struct {
         const n = try protocol.encodeFrame(&buf, msg_type, payload);
         var sent: usize = 0;
         while (sent < n) {
-            const w = try posix.write(cs.fd, buf[sent..n]);
-            if (w == 0) return error.BrokenPipe;
+            const w = posix.write(cs.fd, buf[sent..n]) catch |err| {
+                // WouldBlock before any data sent = transient backpressure, skip frame
+                // WouldBlock after partial send = protocol stream corrupted, must disconnect
+                if (err == error.WouldBlock and sent == 0) return err;
+                cs.write_failed = true;
+                return err;
+            };
+            if (w == 0) {
+                cs.write_failed = true;
+                return error.BrokenPipe;
+            }
             sent += w;
         }
     }
@@ -1213,6 +1250,50 @@ pub const Server = struct {
         }
     }
 
+    // ── sweepFailedClients ─────────────────────────────────────────────
+
+    /// Disconnect clients whose write_failed flag is set (protocol stream corrupted).
+    /// Safe to call after iteration since it collects IDs first.
+    fn sweepFailedClients(self: *Server) void {
+        var to_remove: [MAX_CLIENTS]u16 = undefined;
+        var count: u8 = 0;
+        var it = self.clients.iterator();
+        while (it.next()) |entry| {
+            if (entry.value_ptr.*.write_failed) {
+                if (count < MAX_CLIENTS) {
+                    to_remove[count] = entry.key_ptr.*;
+                    count += 1;
+                }
+            }
+        }
+        for (to_remove[0..count]) |cid| {
+            self.disconnectClient(cid);
+        }
+    }
+
+    // ── cleanupClientStateForTab ────────────────────────────────────────
+
+    /// Remove scroll_offsets entries for all panes in the given tab from every client.
+    fn cleanupClientStateForTab(self: *Server, tab_idx: u8) void {
+        const tab = self.tab_manager.activeTab(tab_idx);
+        var cit = self.clients.valueIterator();
+        while (cit.next()) |client_cs| {
+            self.removeScrollOffsetsInNode(client_cs.*, tab.pane_tree.root);
+        }
+    }
+
+    fn removeScrollOffsetsInNode(self: *Server, cs_ptr: *ClientState, node: *pane_mod.LayoutNode) void {
+        switch (node.*) {
+            .leaf => |l| {
+                _ = cs_ptr.scroll_offsets.remove(l.id);
+            },
+            .split => |s| {
+                self.removeScrollOffsetsInNode(cs_ptr, s.first);
+                self.removeScrollOffsetsInNode(cs_ptr, s.second);
+            },
+        }
+    }
+
     // ── destroyAllPanesInTab ─────────────────────────────────────────────
 
     fn destroyAllPanesInTab(self: *Server, tab_idx: u8) void {