a73x

7a72f5a7

Account for ssh's temporary control-socket name in the path budget

a73x   2026-09-06 08:48

Commit message
Account for ssh's temporary control-socket name in the path budget
src/ssh_share.rs
Old New
@@ -79,11 +79,28 @@ impl SharedSsh {
79 /// long `TMPDIR` into a sync that cannot fetch at all. 79 /// long `TMPDIR` into a sync that cannot fetch at all.
80 const SOCKET_PATH_LIMIT: usize = 100; 80 const SOCKET_PATH_LIMIT: usize = 100;
81 81
82 /// `%C` in a ControlPath expands to a 40-character hash of host, port and
83 /// user, which is why it is used: the name is a fixed length whatever the
84 /// remote is called.
85 const CONTROL_PATH_HASH: usize = 40;
86
87 /// What ssh adds to that name while it brings the master up: a `.` and 16
88 /// random characters, listened on and then renamed onto the real path.
89 ///
90 /// The budget has to include it, because the temporary name is the one that
91 /// has to fit in `sun_path` — and it is 17 bytes longer than the path we
92 /// chose. Leaving it out (until 2026-09-06) let a 59-byte directory pass a
93 /// budget of exactly 100 and then hand ssh a 117-byte path, so `ssh` refused
94 /// with `unix_listener: path "…" too long for Unix domain socket` and the
95 /// fetch exited 128. That is worse than not sharing a connection: the whole
96 /// sync fails. `tests/sync_ssh_connection_test.rs` is the end-to-end check.
97 const SSH_MASTER_TEMP_SUFFIX: usize = ".".len() + 16;
98
82 /// Where the control sockets go, or `None` when a socket under it could not 99 /// Where the control sockets go, or `None` when a socket under it could not
83 /// be opened. `%C` expands to a 40-character hash. 100 /// be opened — in which case the caller simply does not share a connection.
84 fn socket_dir(tmp: &std::path::Path, pid: u32) -> Option<PathBuf> { 101 fn socket_dir(tmp: &std::path::Path, pid: u32) -> Option<PathBuf> {
85 let dir = tmp.join(format!("git-collab-ssh-{pid}")); 102 let dir = tmp.join(format!("git-collab-ssh-{pid}"));
86 let socket_len = dir.as_os_str().len() + "/".len() + 40; 103 let socket_len = dir.as_os_str().len() + "/".len() + CONTROL_PATH_HASH + SSH_MASTER_TEMP_SUFFIX;
87 (socket_len <= SOCKET_PATH_LIMIT).then_some(dir) 104 (socket_len <= SOCKET_PATH_LIMIT).then_some(dir)
88 } 105 }
89 106
@@ -119,4 +136,36 @@ mod tests {
119 let dir = socket_dir(std::path::Path::new("/tmp"), 42).unwrap(); 136 let dir = socket_dir(std::path::Path::new("/tmp"), 42).unwrap();
120 assert_eq!(dir, std::path::Path::new("/tmp/git-collab-ssh-42")); 137 assert_eq!(dir, std::path::Path::new("/tmp/git-collab-ssh-42"));
121 } 138 }
139
140 /// The boundary the old budget got wrong: this directory is 59 bytes, so
141 /// path + `/` + `%C` came to exactly the 100-byte limit and was accepted,
142 /// but the name ssh actually listens on is 117 bytes and it refused to
143 /// start. Refusing to share here is correct; failing the sync was not.
144 #[test]
145 fn a_dir_that_only_fits_without_sshs_temporary_suffix_is_refused() {
146 let tmp = std::path::Path::new("/tmp/delta-terminal-.delta-fs-foQjAw");
147 let dir = tmp.join("git-collab-ssh-3571731");
148 assert_eq!(dir.as_os_str().len(), 59, "the case this pins moved");
149 assert_eq!(dir.as_os_str().len() + 1 + CONTROL_PATH_HASH, 100);
150 assert!(socket_dir(tmp, 3571731).is_none());
151 }
152
153 /// And the largest directory that still leaves room for the real name,
154 /// derived from the constants rather than counted by hand.
155 #[test]
156 fn the_largest_dir_that_fits_the_whole_socket_name_is_accepted() {
157 let room_for_dir =
158 SOCKET_PATH_LIMIT - "/".len() - CONTROL_PATH_HASH - SSH_MASTER_TEMP_SUFFIX;
159 // socket_dir appends "/git-collab-ssh-1" to whatever it is given.
160 let appended = "/git-collab-ssh-1".len();
161 let tmp = std::path::Path::new("/").join("x".repeat(room_for_dir - appended - "/".len()));
162
163 let dir = socket_dir(&tmp, 1).unwrap();
164 assert_eq!(dir.as_os_str().len(), room_for_dir);
165 assert_eq!(
166 dir.as_os_str().len() + "/".len() + CONTROL_PATH_HASH + SSH_MASTER_TEMP_SUFFIX,
167 SOCKET_PATH_LIMIT,
168 "this case must sit exactly on the limit"
169 );
170 }
122 } 171 }
tests/sync_ssh_connection_test.rs
Old New
@@ -29,10 +29,21 @@ fn a_sync_authenticates_to_the_server_once() {
29 -o IdentitiesOnly=yes -o BatchMode=yes -o ConnectTimeout=5", 29 -o IdentitiesOnly=yes -o BatchMode=yes -o ConnectTimeout=5",
30 key.display() 30 key.display()
31 ); 31 );
32
33 // Sharing a connection means a control socket under `TMPDIR`, and a Unix
34 // socket path is capped around 104 bytes. Declining to share under a long
35 // `TMPDIR` is correct behaviour (see `ssh_share::socket_dir`), so a test
36 // that inherited the ambient one would assert sharing on some machines and
37 // fail on others — CI containers and Delta worktrees both hand out temp
38 // paths long enough to trip it. Pin a short root so this test measures the
39 // sharing behaviour and not the length of the environment's temp path.
40 let short_tmp = tempfile::TempDir::new_in("/tmp").expect("short temp dir under /tmp");
41
32 let output = repo 42 let output = repo
33 .cli_command() 43 .cli_command()
34 .args(["sync", "--remote", "srv"]) 44 .args(["sync", "--remote", "srv"])
35 .env("GIT_SSH_COMMAND", &ssh) 45 .env("GIT_SSH_COMMAND", &ssh)
46 .env("TMPDIR", short_tmp.path())
36 .output() 47 .output()
37 .expect("failed to run git-collab sync"); 48 .expect("failed to run git-collab sync");
38 assert!( 49 assert!(