feat: Mac port #183

HACKING.md

@@ -212,3 +212,20 @@ leave log files in place you might run
 ```
 $ SHPOOL_LEAVE_TEST_LOGS=true cargo test --test attach happy_path -- --nocapture
 ```
+
+## Running Tests on macOS
+
+On macOS, some tests in the `config_watcher` module that rely on file
+system events can be flaky when run in parallel. This is because macOS
+uses FSEvents for file system notifications, which delivers events
+asynchronously and with less predictable timing under concurrent load
+compared to Linux's inotify.
+
+To run the tests reliably on macOS, use single-threaded execution:
+
+```
+$ cargo test -- --test-threads=1
+```
+
+This ensures tests run serially and don't interfere with each other's
+file system event delivery.

libshpool/Cargo.toml

@@ -28,7 +28,7 @@ serde_json = "1" # JSON output for list command
 toml = "0.8" # config parsing
 byteorder = "1" # endianness
 signal-hook = "0.3" # signal handling
-shpool_pty = "0.3.1" # spawning shells in ptys
+shpool_pty = "0.3.2" # spawning shells in ptys
 lazy_static = "1" # globals
 crossbeam-channel = "0.5" # channels
 libc = "0.2" # basic libc types

libshpool/src/config.rs

@@ -15,7 +15,7 @@
 use std::{
     borrow::Cow,
     collections::HashMap,
-    env, fs,
+    fs,
     path::{Path, PathBuf},
     sync::{Arc, RwLock, RwLockReadGuard},
 };
@@ -155,7 +155,7 @@ impl Manager {
 
     #[cfg(not(any(target_os = "macos", target_os = "ios", target_os = "windows")))]
     fn config_base_dir() -> anyhow::Result<PathBuf> {
-        match env::var("XDG_CONFIG_DIR") {
+        match std::env::var("XDG_CONFIG_DIR") {
             Ok(v) => Ok(PathBuf::from(v)),
             Err(_) => {
                 let user_info = user::info().context("getting user info")?;

libshpool/src/config_watcher.rs

@@ -15,7 +15,7 @@
 use anyhow::{anyhow, Context as _, Result};
 use crossbeam_channel::{bounded, select, unbounded, Receiver, Sender};
 use notify::{
-    event::ModifyKind, recommended_watcher, Event, EventKind, INotifyWatcher, RecursiveMode,
+    event::ModifyKind, recommended_watcher, Event, EventKind, RecommendedWatcher, RecursiveMode,
     Watcher as _,
 };
 use std::{
@@ -28,10 +28,36 @@ use tracing::{debug, error, instrument, warn};
 
 use crate::test_hooks;
 
+/// Canonicalize a path, resolving symlinks in the existing portion.
+///
+/// This is needed because file system watchers (inotify, FSEvents, etc.) report
+/// canonical paths, so we need to store canonical paths for comparison.
+/// Unlike `std::fs::canonicalize`, this handles paths where the final
+/// components don't exist yet by canonicalizing the longest existing prefix.
+fn canonicalize_path(path: &Path) -> PathBuf {
+    // Try to canonicalize the whole path first
+    if let Ok(canonical) = path.canonicalize() {
+        return canonical;
+    }
+
+    // Find the longest existing ancestor and canonicalize that
+    for ancestor in path.ancestors().skip(1) {
+        if let Ok(canonical_ancestor) = ancestor.canonicalize() {
+            // Append the remaining (non-existent) components
+            if let Ok(remaining) = path.strip_prefix(ancestor) {
+                return canonical_ancestor.join(remaining);
+            }
+        }
+    }
+
+    // Fallback to original path if nothing could be canonicalized
+    path.to_path_buf()
+}
+
 /// Watches on `path`, returnes the watched path, which is the closest existing
 /// ancestor of `path`, and the immediate child that is of interest.
 pub fn best_effort_watch<'a>(
-    watcher: &mut INotifyWatcher,
+    watcher: &mut RecommendedWatcher,
     path: &'a Path,
 ) -> Result<(&'a Path, Option<&'a Path>)> {
     let mut watched_path = Err(anyhow!("empty path"));
@@ -206,7 +232,7 @@ struct ConfigWatcherInner<Handler> {
     handler: Handler,
 
     /// underlying notify-rs watcher
-    watcher: INotifyWatcher,
+    watcher: RecommendedWatcher,
     /// receiving notify events
     notify_rx: Receiver<Result<notify::Event, notify::Error>>,
 
@@ -273,8 +299,11 @@ impl<Handler> ConfigWatcherInner<Handler> {
                 return Outcome::Timeout;
             }
 
-            // nothing ready to act immediately, notify debug_tx
-            self.debug_tx.send(()).unwrap();
+            // Only signal idle if there's no pending reload deadline.
+            // If there's a pending deadline, we have work to do (wait for timeout).
+            if self.reload_deadline.is_none() {
+                self.debug_tx.send(()).unwrap();
+            }
         }
 
         // finally blocking wait
@@ -299,7 +328,8 @@ impl<Handler> ConfigWatcherInner<Handler> {
 
     /// Handle add watch command from `ConfigWatcher`.
     fn add_watch_by_command(&mut self, path: PathBuf) -> Result<()> {
-        match self.paths.entry(path) {
+        let canonical_path = canonicalize_path(&path);
+        match self.paths.entry(canonical_path) {
             Entry::Occupied(e) => Err(anyhow!("{} is already being watched", e.key().display())),
             e @ Entry::Vacant(_) => {
                 let reload = watch_and_add(&mut self.watcher, e)?;
@@ -430,13 +460,16 @@ fn handle_event(event: Event, paths: &HashMap<PathBuf, (PathBuf, PathBuf)>) -> (
 /// failed. Note that this will overwrite any existing state.
 /// Return whether reload is needed.
 fn watch_and_add(
-    watcher: &mut INotifyWatcher,
+    watcher: &mut RecommendedWatcher,
     entry: Entry<PathBuf, (PathBuf, PathBuf)>,
 ) -> Result<bool> {
     // make a version of watch path that doesn't retain a borrow in its return value
-    let best_effort_watch_owned = |watcher: &mut INotifyWatcher, path: &Path| {
-        best_effort_watch(watcher, path)
-            .map(|(w, ic)| (w.to_owned(), w.join(ic.unwrap_or_else(|| Path::new("")))))
+    let best_effort_watch_owned = |watcher: &mut RecommendedWatcher, path: &Path| {
+        best_effort_watch(watcher, path).map(|(w, ic)| {
+            let watched = w.canonicalize().unwrap_or_else(|_| w.to_path_buf());
+            let immediate = watched.join(ic.unwrap_or_else(|| Path::new("")));
+            (watched, immediate)
+        })
     };
     match best_effort_watch_owned(watcher, entry.key()) {
         Ok((watched_path, immediate_child_path)) => {
@@ -676,7 +709,6 @@ mod test {
 
     // Wait for watcher to do its work and drop the watcher to close the channel
     fn drop_watcher(watcher: ConfigWatcher) {
-        // sleep time larger than 1 debounce time
         thread::sleep(DEBOUNCE_TIME * 2);
         watcher.worker_ready();
     }
@@ -689,9 +721,8 @@ mod test {
         fs::create_dir_all(state.target_path.parent().unwrap()).unwrap();
 
         state.watcher.worker_ready();
+        // Write twice in quick succession - both should be within debounce window
         fs::write(&state.target_path, "test").unwrap();
-
-        state.watcher.worker_ready();
         fs::write(&state.target_path, "another").unwrap();
 
         drop_watcher(state.watcher);
@@ -736,6 +767,44 @@ mod test {
         drop_watcher(state.watcher);
 
         let reloads: Vec<_> = state.rx.into_iter().collect();
-        assert_eq!(reloads.len(), 1);
+        assert_eq!(reloads.len(), 1, "expected 1 reload, got {}", reloads.len());
+    }
+
+    /// Regression test: ConfigWatcher should resolve symlinks in watched paths.
+    ///
+    /// File system watchers (inotify on Linux, FSEvents on macOS) report
+    /// canonical paths. If we watch through a symlink, we need to canonicalize
+    /// the stored path to match events we receive. Without this, events are
+    /// missed because the symlinked path doesn't match the canonical event path.
+    ///
+    /// This commonly manifests on macOS where /var -> /private/var, but affects
+    /// any platform when symlinks are in the watched path.
+    #[test]
+    #[timeout(30000)]
+    fn symlink_path_is_canonicalized() {
+        use std::os::unix::fs::symlink;
+
+        let tmpdir = tempfile::tempdir().unwrap();
+
+        // setup: real dir + symlink to it
+        let real_dir = tmpdir.path().join("real");
+        fs::create_dir_all(&real_dir).unwrap();
+        let link_dir = tmpdir.path().join("link");
+        symlink(&real_dir, &link_dir).unwrap();
+
+        // watch through the symlink
+        let symlinked_target = link_dir.join("config.toml");
+        let (tx, rx) = unbounded();
+        let watcher =
+            ConfigWatcher::with_debounce(move || tx.send(()).unwrap(), DEBOUNCE_TIME).unwrap();
+        watcher.watch(&symlinked_target).unwrap();
+
+        watcher.worker_ready();
+        fs::write(&symlinked_target, "test content").unwrap();
+
+        drop_watcher(watcher);
+
+        let reloads: Vec<_> = rx.into_iter().collect();
+        assert_eq!(reloads.len(), 1, "expected 1 reload, got {}", reloads.len());
     }
 }

libshpool/src/daemon/prompt.rs

@@ -107,9 +107,11 @@ pub fn maybe_inject_prefix(
     // this rather than `echo $PROMPT_SENTINEL` because different
     // shells have subtly different echo behavior which makes it
     // hard to make the scanner work right.
-    // TODO(julien): this will probably not work on mac
-    let sentinel_cmd =
-        format!("\n {}=prompt /proc/{}/exe daemon\n", SENTINEL_FLAG_VAR, std::process::id());
+    let exe_path = std::env::current_exe()
+        .context("getting current exe path")?
+        .to_string_lossy()
+        .into_owned();
+    let sentinel_cmd = format!("\n {}=prompt {} daemon\n", SENTINEL_FLAG_VAR, exe_path);
     script.push_str(sentinel_cmd.as_str());
 
     debug!("injecting prefix script '{}'", script);
@@ -121,8 +123,11 @@ pub fn maybe_inject_prefix(
 #[instrument(skip_all)]
 fn wait_for_startup(pty_master: &mut shpool_pty::fork::Master) -> anyhow::Result<()> {
     let mut startup_sentinel_scanner = SentinelScanner::new(STARTUP_SENTINEL);
-    let startup_sentinel_cmd =
-        format!("\n {}=startup /proc/{}/exe daemon\n", SENTINEL_FLAG_VAR, std::process::id());
+    let exe_path = std::env::current_exe()
+        .context("getting current exe path")?
+        .to_string_lossy()
+        .into_owned();
+    let startup_sentinel_cmd = format!("\n {}=startup {} daemon\n", SENTINEL_FLAG_VAR, exe_path);
 
     pty_master
         .write_all(startup_sentinel_cmd.as_bytes())

libshpool/src/daemon/server.rs

@@ -144,9 +144,21 @@ impl Server {
     #[instrument(skip_all, fields(cid = conn_id))]
     fn handle_conn(&self, mut stream: UnixStream, conn_id: usize) -> anyhow::Result<()> {
         // We want to avoid timing out while blocking the main thread.
+        // On macOS, set_read_timeout returns EINVAL if the peer has already
+        // closed (e.g., a daemon presence probe). This is documented in the
+        // macOS setsockopt(2) man page. Treat this the same as a broken pipe.
+        #[cfg(target_os = "macos")]
+        if let Err(e) = stream.set_read_timeout(Some(consts::SOCK_STREAM_TIMEOUT)) {
+            if e.raw_os_error() == Some(libc::EINVAL) {
+                info!("EINVAL setting read timeout, peer already closed (presence probe)");
+                return Ok(());
+            }
+            return Err(e).context("setting read timeout on inbound session");
+        }
+        #[cfg(not(target_os = "macos"))]
         stream
             .set_read_timeout(Some(consts::SOCK_STREAM_TIMEOUT))
-            .context("setting read timout on inbound session")?;
+            .context("setting read timeout on inbound session")?;
 
         // advertize our protocol version to the client so that it can
         // warn about mismatches
@@ -1138,12 +1150,14 @@ where
     protocol::encode_to(&header, serializeable_stream).context("writing reply")?;
 
     stream.set_write_timeout(None).context("unsetting write timout on inbound session")?;
+
     Ok(())
 }
 
 /// check_peer makes sure that a process dialing in on the shpool
 /// control socket has the same UID as the current user and that
 /// both have the same executable path.
+#[cfg(target_os = "linux")]
 fn check_peer(sock: &UnixStream) -> anyhow::Result<()> {
     use nix::sys::socket;
 
@@ -1166,11 +1180,69 @@ fn check_peer(sock: &UnixStream) -> anyhow::Result<()> {
     Ok(())
 }
 
+#[cfg(target_os = "macos")]
+fn check_peer(sock: &UnixStream) -> anyhow::Result<()> {
+    use std::os::unix::io::AsRawFd;
+
+    let mut peer_uid: libc::uid_t = 0;
+    let mut peer_gid: libc::gid_t = 0;
+    unsafe {
+        if libc::getpeereid(sock.as_raw_fd(), &mut peer_uid, &mut peer_gid) != 0 {
+            return Err(anyhow!(
+                "could not get peer uid from socket: {}",
+                io::Error::last_os_error()
+            ));
+        }
+    }
+    let peer_uid = unistd::Uid::from_raw(peer_uid);
+    let self_uid = unistd::Uid::current();
+    if peer_uid != self_uid {
+        return Err(anyhow!("shpool prohibits connections across users"));
+    }
+
+    let mut peer_pid: libc::pid_t = 0;
+    let mut len = std::mem::size_of::<libc::pid_t>() as libc::socklen_t;
+    unsafe {
+        if libc::getsockopt(
+            sock.as_raw_fd(),
+            libc::SOL_LOCAL,
+            libc::LOCAL_PEERPID,
+            &mut peer_pid as *mut _ as *mut libc::c_void,
+            &mut len,
+        ) != 0
+        {
+            return Err(anyhow!(
+                "could not get peer pid from socket: {}",
+                io::Error::last_os_error()
+            ));
+        }
+    }
+
+    let peer_pid = unistd::Pid::from_raw(peer_pid);
+    let self_pid = unistd::Pid::this();
+    let peer_exe = exe_for_pid(peer_pid).context("could not resolve exe from the pid")?;
+    let self_exe = exe_for_pid(self_pid).context("could not resolve our own exe")?;
+    if peer_exe != self_exe {
+        warn!("attach binary differs from daemon binary");
+    }
+
+    Ok(())
+}
+
+#[cfg(target_os = "linux")]
 fn exe_for_pid(pid: unistd::Pid) -> anyhow::Result<PathBuf> {
     let path = std::fs::read_link(format!("/proc/{pid}/exe"))?;
     Ok(path)
 }
 
+#[cfg(target_os = "macos")]
+fn exe_for_pid(pid: unistd::Pid) -> anyhow::Result<PathBuf> {
+    use libproc::proc_pid::pidpath;
+    let path = pidpath(pid.as_raw())
+        .map_err(|e| anyhow!("could not get exe path for pid {}: {:?}", pid, e))?;
+    Ok(PathBuf::from(path))
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum ShellSelectionError {
     BusyShellSession,

libshpool/src/user.rs

@@ -33,6 +33,12 @@ pub fn info() -> anyhow::Result<Info> {
         pw_gecos: ptr::null_mut(),
         pw_dir: ptr::null_mut(),
         pw_shell: ptr::null_mut(),
+        #[cfg(any(target_os = "macos", target_os = "ios", target_os = "freebsd", target_os = "netbsd", target_os = "openbsd"))]
+        pw_change: 0,
+        #[cfg(any(target_os = "macos", target_os = "ios", target_os = "freebsd", target_os = "netbsd", target_os = "openbsd"))]
+        pw_class: ptr::null_mut(),
+        #[cfg(any(target_os = "macos", target_os = "ios", target_os = "freebsd", target_os = "netbsd", target_os = "openbsd"))]
+        pw_expire: 0,
     };
     let mut passwd_res_ptr: *mut libc::passwd = ptr::null_mut();
     unsafe {

shpool/Cargo.toml

@@ -15,6 +15,9 @@ rust-version = "1.74"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
+[features]
+test_hooks = ["libshpool/test_hooks"]
+
 [dependencies]
 clap = { version = "4", features = ["derive"] } # cli parsing
 anyhow = "1" # dynamic, unstructured errors