fix(sandbox): track PTY state per SSH channel to fix terminal resize (#687)

johntmyers · web-flow · commit ed74a19a6112 · 2026-03-30T16:08:26.000-07:00
Replace flat pty_master/input_sender/pty_request fields in SshHandler with a HashMap<ChannelId, ChannelState> so each channel tracks its own PTY resources independently. This fixes window_change_request resizing the wrong PTY when multiple channels are open simultaneously. Also fixes ioctl UB in set_winsize (pass &winsize not winsize by value) and adds warn! logging for unknown channels across all handlers. Resolves #543
diff --git a/crates/openshell-sandbox/src/ssh.rs b/crates/openshell-sandbox/src/ssh.rs
@@ -263,16 +263,27 @@ fn hmac_sha256(key: &[u8], data: &[u8]) -> String {
     hex::encode(result)
 }
 
+/// Per-channel state for tracking PTY resources and I/O senders.
+///
+/// Each SSH channel gets its own PTY master (if a PTY was requested) and input
+/// sender.  This allows `window_change_request` to resize the correct PTY when
+/// multiple channels are open simultaneously (e.g. parallel shells, shell +
+/// sftp, etc.).
+#[derive(Default)]
+struct ChannelState {
+    input_sender: Option<mpsc::Sender<Vec<u8>>>,
+    pty_master: Option<std::fs::File>,
+    pty_request: Option<PtyRequest>,
+}
+
 struct SshHandler {
     policy: SandboxPolicy,
     workdir: Option<String>,
     netns_fd: Option<RawFd>,
     proxy_url: Option<String>,
     ca_file_paths: Option<Arc<(PathBuf, PathBuf)>>,
     provider_env: HashMap<String, String>,
-    input_sender: Option<mpsc::Sender<Vec<u8>>>,
-    pty_master: Option<std::fs::File>,
-    pty_request: Option<PtyRequest>,
+    channels: HashMap<ChannelId, ChannelState>,
 }
 
 impl SshHandler {
@@ -291,9 +302,7 @@ impl SshHandler {
             proxy_url,
             ca_file_paths,
             provider_env,
-            input_sender: None,
-            pty_master: None,
-            pty_request: None,
+            channels: HashMap::new(),
         }
     }
 }
@@ -315,12 +324,27 @@ impl russh::server::Handler for SshHandler {
 
     async fn channel_open_session(
         &mut self,
-        _channel: russh::Channel<russh::server::Msg>,
+        channel: russh::Channel<russh::server::Msg>,
         _session: &mut Session,
     ) -> Result<bool, Self::Error> {
+        self.channels.insert(channel.id(), ChannelState::default());
         Ok(true)
     }
 
+    /// Clean up per-channel state when the channel is closed.
+    ///
+    /// This is the final cleanup and subsumes `channel_eof` — if `channel_close`
+    /// fires without a preceding `channel_eof`, all resources (pty_master File,
+    /// input_sender) are dropped here.
+    async fn channel_close(
+        &mut self,
+        channel: ChannelId,
+        _session: &mut Session,
+    ) -> Result<(), Self::Error> {
+        self.channels.remove(&channel);
+        Ok(())
+    }
+
     async fn channel_open_direct_tcpip(
         &mut self,
         channel: russh::Channel<russh::server::Msg>,
@@ -388,7 +412,11 @@ impl russh::server::Handler for SshHandler {
         _modes: &[(russh::Pty, u32)],
         session: &mut Session,
     ) -> Result<(), Self::Error> {
-        self.pty_request = Some(PtyRequest {
+        let state = self
+            .channels
+            .get_mut(&channel)
+            .ok_or_else(|| anyhow::anyhow!("pty_request on unknown channel {channel:?}"))?;
+        state.pty_request = Some(PtyRequest {
             term: term.to_string(),
             col_width,
             row_height,
@@ -401,21 +429,27 @@ impl russh::server::Handler for SshHandler {
 
     async fn window_change_request(
         &mut self,
-        _channel: ChannelId,
+        channel: ChannelId,
         col_width: u32,
         row_height: u32,
         pixel_width: u32,
         pixel_height: u32,
         _session: &mut Session,
     ) -> Result<(), Self::Error> {
-        if let Some(master) = self.pty_master.as_ref() {
+        let Some(state) = self.channels.get(&channel) else {
+            warn!("window_change_request on unknown channel {channel:?}");
+            return Ok(());
+        };
+        if let Some(master) = state.pty_master.as_ref() {
             let winsize = Winsize {
                 ws_row: to_u16(row_height.max(1)),
                 ws_col: to_u16(col_width.max(1)),
                 ws_xpixel: to_u16(pixel_width),
                 ws_ypixel: to_u16(pixel_height),
             };
-            let _ = unsafe_pty::set_winsize(master.as_raw_fd(), winsize);
+            if let Err(e) = unsafe_pty::set_winsize(master.as_raw_fd(), winsize) {
+                warn!("failed to resize PTY for channel {channel:?}: {e}");
+            }
         }
         Ok(())
     }
@@ -474,7 +508,10 @@ impl russh::server::Handler for SshHandler {
                 self.ca_file_paths.clone(),
                 &self.provider_env,
             )?;
-            self.input_sender = Some(input_sender);
+            let state = self.channels.get_mut(&channel).ok_or_else(|| {
+                anyhow::anyhow!("subsystem_request on unknown channel {channel:?}")
+            })?;
+            state.input_sender = Some(input_sender);
         } else {
             warn!(subsystem = name, "unsupported subsystem requested");
             session.channel_failure(channel)?;
@@ -499,26 +536,34 @@ impl russh::server::Handler for SshHandler {
 
     async fn data(
         &mut self,
-        _channel: ChannelId,
+        channel: ChannelId,
         data: &[u8],
         _session: &mut Session,
     ) -> Result<(), Self::Error> {
-        if let Some(sender) = self.input_sender.as_ref() {
+        let Some(state) = self.channels.get(&channel) else {
+            warn!("data on unknown channel {channel:?}");
+            return Ok(());
+        };
+        if let Some(sender) = state.input_sender.as_ref() {
             let _ = sender.send(data.to_vec());
         }
         Ok(())
     }
 
     async fn channel_eof(
         &mut self,
-        _channel: ChannelId,
+        channel: ChannelId,
         _session: &mut Session,
     ) -> Result<(), Self::Error> {
         // Drop the input sender so the stdin writer thread sees a
         // disconnected channel and closes the child's stdin pipe.  This
         // is essential for commands like `cat | tar xf -` which need
         // stdin EOF to know the input stream is complete.
-        self.input_sender.take();
+        if let Some(state) = self.channels.get_mut(&channel) {
+            state.input_sender.take();
+        } else {
+            warn!("channel_eof on unknown channel {channel:?}");
+        }
         Ok(())
     }
 }
@@ -530,7 +575,11 @@ impl SshHandler {
         handle: Handle,
         command: Option<String>,
     ) -> anyhow::Result<()> {
-        if let Some(pty) = self.pty_request.take() {
+        let state = self
+            .channels
+            .get_mut(&channel)
+            .ok_or_else(|| anyhow::anyhow!("start_shell on unknown channel {channel:?}"))?;
+        if let Some(pty) = state.pty_request.take() {
             // PTY was requested — allocate a real PTY (interactive shell or
             // exec that explicitly asked for a terminal).
             let (pty_master, input_sender) = spawn_pty_shell(
@@ -545,8 +594,8 @@ impl SshHandler {
                 self.ca_file_paths.clone(),
                 &self.provider_env,
             )?;
-            self.pty_master = Some(pty_master);
-            self.input_sender = Some(input_sender);
+            state.pty_master = Some(pty_master);
+            state.input_sender = Some(input_sender);
         } else {
             // No PTY requested — use plain pipes so stdout/stderr are
             // separate and output has clean LF line endings.  This is the
@@ -562,7 +611,7 @@ impl SshHandler {
                 self.ca_file_paths.clone(),
                 &self.provider_env,
             )?;
-            self.input_sender = Some(input_sender);
+            state.input_sender = Some(input_sender);
         }
         Ok(())
     }
@@ -999,7 +1048,7 @@ mod unsafe_pty {
 
     #[allow(unsafe_code)]
     pub fn set_winsize(fd: RawFd, winsize: Winsize) -> std::io::Result<()> {
-        let rc = unsafe { libc::ioctl(fd, libc::TIOCSWINSZ, winsize) };
+        let rc = unsafe { libc::ioctl(fd, libc::TIOCSWINSZ, &winsize) };
         if rc != 0 {
             return Err(std::io::Error::last_os_error());
         }
@@ -1404,4 +1453,111 @@ mod tests {
         assert!(!is_loopback_host("not-an-ip"));
         assert!(!is_loopback_host("[]"));
     }
+
+    // -----------------------------------------------------------------------
+    // Per-channel PTY state tests (#543)
+    // -----------------------------------------------------------------------
+
+    #[test]
+    fn set_winsize_applies_to_correct_pty() {
+        // Verify that set_winsize applies to a specific PTY master FD,
+        // which is the mechanism that per-channel tracking relies on.
+        // With the old single-pty_master design, a window_change_request
+        // for channel N would resize whatever PTY was stored last —
+        // potentially belonging to a different channel.
+        let pty_a = openpty(None, None).expect("openpty a");
+        let pty_b = openpty(None, None).expect("openpty b");
+        let master_a = std::fs::File::from(pty_a.master);
+        let master_b = std::fs::File::from(pty_b.master);
+        let fd_a = master_a.as_raw_fd();
+        let fd_b = master_b.as_raw_fd();
+        assert_ne!(fd_a, fd_b, "two PTYs must have distinct FDs");
+
+        // Close the slave ends to avoid leaking FDs in the test.
+        drop(std::fs::File::from(pty_a.slave));
+        drop(std::fs::File::from(pty_b.slave));
+
+        // Resize only PTY B.
+        let winsize_b = Winsize {
+            ws_row: 50,
+            ws_col: 120,
+            ws_xpixel: 0,
+            ws_ypixel: 0,
+        };
+        unsafe_pty::set_winsize(fd_b, winsize_b).expect("set_winsize on PTY B");
+
+        // Resize PTY A to a different size.
+        let winsize_a = Winsize {
+            ws_row: 24,
+            ws_col: 80,
+            ws_xpixel: 0,
+            ws_ypixel: 0,
+        };
+        unsafe_pty::set_winsize(fd_a, winsize_a).expect("set_winsize on PTY A");
+
+        // Read back sizes via ioctl to verify independence.
+        let mut actual_a: libc::winsize = unsafe { std::mem::zeroed() };
+        let mut actual_b: libc::winsize = unsafe { std::mem::zeroed() };
+        #[allow(unsafe_code)]
+        unsafe {
+            libc::ioctl(fd_a, libc::TIOCGWINSZ, &mut actual_a);
+            libc::ioctl(fd_b, libc::TIOCGWINSZ, &mut actual_b);
+        }
+
+        assert_eq!(actual_a.ws_row, 24, "PTY A should be 24 rows");
+        assert_eq!(actual_a.ws_col, 80, "PTY A should be 80 cols");
+        assert_eq!(actual_b.ws_row, 50, "PTY B should be 50 rows");
+        assert_eq!(actual_b.ws_col, 120, "PTY B should be 120 cols");
+    }
+
+    #[test]
+    fn channel_state_independent_input_senders() {
+        // Verify that each channel gets its own input sender so that
+        // data() and channel_eof() affect only the targeted channel.
+        let (tx_a, rx_a) = mpsc::channel::<Vec<u8>>();
+        let (tx_b, rx_b) = mpsc::channel::<Vec<u8>>();
+
+        let mut state_a = ChannelState {
+            input_sender: Some(tx_a),
+            ..Default::default()
+        };
+        let state_b = ChannelState {
+            input_sender: Some(tx_b),
+            ..Default::default()
+        };
+
+        // Send data to channel A only.
+        state_a
+            .input_sender
+            .as_ref()
+            .unwrap()
+            .send(b"hello-a".to_vec())
+            .unwrap();
+        // Send data to channel B only.
+        state_b
+            .input_sender
+            .as_ref()
+            .unwrap()
+            .send(b"hello-b".to_vec())
+            .unwrap();
+
+        assert_eq!(rx_a.recv().unwrap(), b"hello-a");
+        assert_eq!(rx_b.recv().unwrap(), b"hello-b");
+
+        // EOF on channel A (drop sender) should not affect channel B.
+        state_a.input_sender.take();
+        assert!(
+            rx_a.recv().is_err(),
+            "channel A sender dropped, recv should fail"
+        );
+
+        // Channel B should still be functional.
+        state_b
+            .input_sender
+            .as_ref()
+            .unwrap()
+            .send(b"still-alive".to_vec())
+            .unwrap();
+        assert_eq!(rx_b.recv().unwrap(), b"still-alive");
+    }
 }
