From 858af3d63d5e6fbc0bf766b800de555e27cd080c Mon Sep 17 00:00:00 2001 From: TapTap Date: Wed, 9 Sep 2026 13:41:28 +0200 Subject: [PATCH 1/2] feat(p5-rsh): --rsh/-e, --rsync-path, --blocking-io, --outbuf --- RSYNC_COMPAT.md | 10 +-- src/client/client_cli.c | 43 +++++++++ src/client/client_send.c | 3 +- src/client/usage.c | 9 ++ src/shared/config.c | 4 +- src/shared/config.h | 20 ++++- src/shared/transport_ssh.c | 135 ++++++++++++++++++++++++----- src/shared/transport_ssh.h | 10 ++- tests/integration/test_features.py | 50 +++++++++++ tests/integration/test_ssh.py | 45 +++++++++- tests/test_client_cli.c | 118 ++++++++++++++++++++++++- tests/test_transport_ssh.c | 57 +++++++++++- 12 files changed, 462 insertions(+), 42 deletions(-) diff --git a/RSYNC_COMPAT.md b/RSYNC_COMPAT.md index 5531e52..2257ed9 100644 --- a/RSYNC_COMPAT.md +++ b/RSYNC_COMPAT.md @@ -607,12 +607,12 @@ now transmits targets (the prior behavior was broken/partial); its status moved | Flag | Rsync Description | FastSync Status | Notes | |------|-------------------|-----------------|-------| -| `-e`, `--rsh=COMMAND` | Remote shell to use | ❌ Not Implemented | Removed; SSH invokes `ssh` directly | -| `--rsync-path=PROGRAM` | rsync binary on remote | ❌ Not Implemented | Removed; use `--fastsync-server-path` | +| `-e`, `--rsh=COMMAND` | Remote shell to use | ✅ Implemented | `-e`/`--rsh` (and `--rsh=COMMAND`) select the remote-shell program used to build the SSH child argv, overriding the default `ssh`. The command is whitespace-split into the leading argv words so rsync's `-e "ssh -p 2222"` works; the standard `-o` family, an optional `-p` port, `user@host` and the quoted remote command (`fastsync-server --stdio`) follow. Stored in the `rsh_command` config field. **Client-only, never crosses the wire** (it is a launch concern, not a handshake property) | +| `--rsync-path=PROGRAM` | rsync binary on remote | ✅ Implemented | Alias for `--fastsync-server-path`: both write the `fastsync_server_path` config field used as the remote-side server program (quoted as one remote-shell word unless `--old-args`), which CROSSES the wire as before. Kept separate from `--rsh`, which names the local connecting program | | `--port=PORT` | Alternate daemon port | ✅ Implemented | `server_port` config field | | `--sockopts=OPTIONS` | Custom TCP options | ❌ Not Implemented | | -| `--blocking-io` | Use blocking I/O for remote shell | ❌ Not Implemented | | -| `--outbuf=N\|L\|B` | Set output buffering | ❌ Not Implemented | | +| `--blocking-io` | Use blocking I/O for remote shell | ✅ Implemented | With `--blocking-io` the SSH-transport socketpair socket is left without `SO_RCVTIMEO`/`SO_SNDTIMEO`, so the transfer blocks naturally; by default it gets the same read/write timeout as the TCP transport (see `--timeout`). `blocking_io` config bool. **Client-only, never crosses the wire** | +| `--outbuf=N\|L\|B` | Set output buffering | ✅ Implemented | `N` (none/unbuffered) → `_IONBF`, `L` (line) → `_IOLBF`, `B` (block, the default) → `_IOFBF` via `setvbuf` on stdout and stderr. Garbage values are rejected. `outbuf` config field (`OutbufMode`). **Client-only, never crosses the wire** | | `--address=ADDRESS` | Bind address for outgoing socket | ❌ Not Implemented | Removed because it had no effect | | `-4`, `--ipv4` | Prefer IPv4 | ❌ Not Implemented | Removed because it had no effect | | `-6`, `--ipv6` | Prefer IPv6 | ❌ Not Implemented | Removed because it had no effect | @@ -744,7 +744,7 @@ These options affect process startup, authentication, sockets, and remote execut | Features | Effort | Implementation plan | |----------|--------|--------------------| -| `--rsh=COMMAND`, `-e`; `--rsync-path=PROGRAM`; `--blocking-io`; `--outbuf=N\|L\|B` | M | Generalize SSH command construction and subprocess I/O while retaining argument escaping and timeout guarantees. | +| `--rsh=COMMAND`, `-e`; `--rsync-path=PROGRAM`; `--blocking-io`; `--outbuf=N\|L\|B` | M | ✅ Wave A implemented (see the Connectivity table above). SSH argv construction is generalized: `-e`/`--rsh` replaces the hardcoded `ssh` program (whitespace-split, so `-e "ssh -p 2222"` works), `--rsync-path` aliases the existing `fastsync_server_path`, `--blocking-io` drops the SSH socket timeouts, and `--outbuf` maps N/L/B onto `setvbuf`. All four are client-only launch concerns and never cross the wire. | | `--address=ADDRESS`; `--ipv4`, `-4`; `--ipv6`, `-6`; `--sockopts=OPTIONS`; `--port=PORT` daemon semantics | M | Add explicit socket-family/bind configuration and validate it independently for TCP client and daemon modes. | | `--remote-option=OPT`, `-M`; `--trust-sender` | L | Add authenticated remote-option/config negotiation and reject unsafe sender-controlled values. `-M` conflicts with FastSync metadata mode. | | `--daemon`; `--config=FILE`; `--dparam=OVERRIDE`; `--no-detach`; `--password-file=FILE`; `--early-input=FILE`; `--no-motd` | XL | Implement a real daemon lifecycle, module configuration, authentication, privilege separation, and process management. | diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 2c5b0d2..bf642c7 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -194,6 +194,35 @@ static int set_stderr_mode(const char* value) { return 0; } +/* Parse --outbuf=N|L|B into the config's OutbufMode. N=none (unbuffered), + * L=line-buffered, B=block-buffered (the stdio default). Anything else is a + * clear error, never a silent fallback. */ +static int set_outbuf_option(Config* config, const char* value) { + if (strcmp(value, "N") == 0 || strcmp(value, "n") == 0) + config->outbuf = OUTBUF_NONE; + else if (strcmp(value, "L") == 0 || strcmp(value, "l") == 0) + config->outbuf = OUTBUF_LINE; + else if (strcmp(value, "B") == 0 || strcmp(value, "b") == 0) + config->outbuf = OUTBUF_BLOCK; + else { + log_message(LOG_LEVEL_ERROR, "--outbuf must be N (none), L (line), or B (block)"); + return -1; + } + return 0; +} + +#ifndef FASTSYNC_TEST_BUILD +/* Apply the parsed --outbuf style to stdout/stderr via setvbuf, matching stdio + * semantics: N -> _IONBF (unbuffered), L -> _IOLBF (line), B -> _IOFBF (block, + * the default). */ +static void apply_output_buffering(const Config* config) { + int mode = config->outbuf; + int stdio_mode = (mode == OUTBUF_NONE) ? _IONBF : (mode == OUTBUF_LINE) ? _IOLBF : _IOFBF; + setvbuf(stdout, NULL, stdio_mode, 0); + setvbuf(stderr, NULL, stdio_mode, 0); +} +#endif + static int read_patterns_from_file(const char* filepath, char*** patterns, int* count); static int parse_debug_flags(const char* value, Config* config) { @@ -472,6 +501,8 @@ static const OptionEntry OPTION_TABLE[] = { {"--secluded-args", NULL, OPT_NOOP, 0}, {"--update", "-u", OPT_FLAG, offsetof(Config, update)}, {"--old-args", NULL, OPT_FLAG, offsetof(Config, old_args)}, + {"--rsh", "-e", OPT_STRING, offsetof(Config, rsh_command)}, + {"--blocking-io", NULL, OPT_FLAG, offsetof(Config, blocking_io)}, {"--links", "-l", OPT_FLAG, offsetof(Config, follow_symlinks)}, {"--copy-links", NULL, OPT_FLAG, offsetof(Config, copy_links)}, {"--safe-links", NULL, OPT_FLAG, offsetof(Config, safe_links)}, @@ -521,6 +552,9 @@ static const OptionEntry OPTION_TABLE[] = { {"--ca", NULL, OPT_STRING, offsetof(Config, tls_ca)}, {"--backup-dir", NULL, OPT_STRING, offsetof(Config, backup_dir)}, {"--fastsync-server-path", NULL, OPT_STRING, offsetof(Config, fastsync_server_path)}, + /* --rsync-path is rsync's spelling for the same "server program path"; it + * is a pure alias for fastsync_server_path (never a distinct field). */ + {"--rsync-path", NULL, OPT_STRING, offsetof(Config, fastsync_server_path)}, {"--temp-dir", NULL, OPT_STRING, offsetof(Config, temp_dir)}, {"--partial-dir", NULL, OPT_STRING, offsetof(Config, partial_dir)}, {"--suffix", NULL, OPT_STRING, offsetof(Config, suffix)}, @@ -1191,6 +1225,12 @@ int parse_args(Config* config, int argc, char* argv[], int* positional_args, if (identity_parse_chown(config, argv[++i]) != 0) return -1; config->use_metadata = true; + } else if (strncmp(argv[i], "--outbuf=", 9) == 0) { + if (set_outbuf_option(config, argv[i] + 9) != 0) + return -1; + } else if (opt_is(argv[i], "--outbuf", NULL)) { + if (i + 1 >= argc || set_outbuf_option(config, argv[++i]) != 0) + return -1; } else if (argv[i][0] == '-') { char* escaped = output_escape(argv[i], false); fprintf(stderr, "Unknown option: %s\n", escaped ? escaped : ""); @@ -1395,6 +1435,9 @@ int main(int argc, char* argv[]) { goto cleanup; } + /* Apply the requested --outbuf style now that the mode is parsed. */ + apply_output_buffering(config); + /* --open-noatime is a sender-side policy: install it for every source read (scan + data path) without touching the receiver. */ file_set_open_noatime(config->open_noatime); diff --git a/src/client/client_send.c b/src/client/client_send.c index 2f0c00a..da196e8 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -367,7 +367,8 @@ static Client* connect_transfer_client(const Config* config) { return NULL; } return client_connect_ssh(config->ssh_destination, config->ssh_port, - config->fastsync_server_path, config->old_args); + config->fastsync_server_path, config->old_args, config->rsh_command, + config->blocking_io); } Client* client = client_create(); diff --git a/src/client/usage.c b/src/client/usage.c index 58c2396..f23edb2 100644 --- a/src/client/usage.c +++ b/src/client/usage.c @@ -20,6 +20,15 @@ void print_usage(void) { printf(" -n, --dry-run Show what would be transferred\n"); printf(" --remove-source-files Remove regular source files after successful transfer\n"); printf(" -p SSH port (default: 22)\n"); + printf(" -e, --rsh Remote shell to launch on the client for the SSH\n"); + printf(" transport (default: ssh). The command may include\n"); + printf(" arguments, e.g. -e \"ssh -p 2222\"\n"); + printf(" --rsync-path Alias for --fastsync-server-path (path to the\n"); + printf(" fastsync server binary on the remote side)\n"); + printf(" --blocking-io Leave the SSH transport socket without read/write\n"); + printf(" timeouts so it blocks naturally\n"); + printf(" --outbuf=MODE stdout/stderr buffering: N (none/unbuffered),\n"); + printf(" L (line-buffered), or B (block-buffered, default)\n"); printf(" --progress Show transfer progress\n"); printf(" -P Partial mode with progress (retention incomplete)\n"); printf(" -8, --8-bit-output Leave high-bit characters unescaped in output\n"); diff --git a/src/shared/config.c b/src/shared/config.c index 20ffc27..ebe785e 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -119,7 +119,8 @@ static void config_set_defaults(Config* config) { config->dirs = false; config->mkpath = false; config->rsh_command = NULL; - config->rsync_path = NULL; + config->blocking_io = false; + config->outbuf = OUTBUF_BLOCK; config->old_args = false; config->temp_dir = NULL; config->basis_dirs = NULL; @@ -393,7 +394,6 @@ void config_delete(Config* config) { free(config->files_from); file_list_destroy((FileListSet*)config->files_from_set); free(config->rsh_command); - free(config->rsync_path); free(config->temp_dir); for (int i = 0; i < config->basis_count; i++) { free(config->basis_dirs[i].path); diff --git a/src/shared/config.h b/src/shared/config.h index 6bc9619..698fc3e 100644 --- a/src/shared/config.h +++ b/src/shared/config.h @@ -9,6 +9,15 @@ typedef enum { TRANSPORT_TCP, TRANSPORT_SSH } TransportType; +/* --outbuf stdout/stderr buffering style (client-only launch concern, never + * crosses the wire). OUTBUF_BLOCK is the default, matching the stdio default + * (fully buffered when output is not a terminal). */ +typedef enum { + OUTBUF_BLOCK = 0, /* _IOFBF */ + OUTBUF_LINE, /* _IOLBF */ + OUTBUF_NONE /* _IONBF */ +} OutbufMode; + /* Receiver-side staging state for --delay-updates. Forward-declared here so Config can carry it; the concrete type lives in delay_updates.h. */ typedef struct DelayUpdatesContext DelayUpdatesContext; @@ -226,8 +235,17 @@ typedef struct Config { bool mkpath; // Issue #130: Remote shell/connection options + /* -e/--rsh: the remote-shell program used to establish the SSH transport. + * NULL means the default "ssh". Client-only launch concern: NEVER crosses + * the wire (it is not meaningful to the daemon/server handshake). */ char* rsh_command; - char* rsync_path; + /* --blocking-io: leave the SSH transport socket without + * SO_RCVTIMEO/SO_SNDTIMEO so it blocks naturally instead of timing out. + * Client-only launch concern: NEVER crosses the wire. */ + bool blocking_io; + /* --outbuf mode (OutbufMode): stdout/stderr buffering. Client-only launch + * concern: NEVER crosses the wire. */ + int outbuf; bool old_args; char* temp_dir; /* Alternate basis directories, ordered by command-line appearance. Each diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index d63d920..fe3c18e 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -16,7 +17,9 @@ typedef struct { char* remote_path; } RemoteDest; -static void ssh_child_setup_failed(int status_fd) { +/* Writes the exec-failure marker and exits the child. Marked noreturn so + * static analyzers prove the caller's error path never falls through. */ +__attribute__((noreturn)) static void ssh_child_setup_failed(int status_fd) { ssize_t wret = write(status_fd, "x", 1); (void)wret; _exit(1); @@ -119,8 +122,97 @@ char* ssh_build_remote_command(const char* server_path, bool old_args) { return command; } +/* A heap-owned, NULL-terminated argv whose every string is separately malloc'd + * (str_dup'd) so a caller can free arbitrary slots, including argv[0]. */ +char** ssh_build_client_argv(const char* rsh_command, int port, const char* userhost, + const char* remote_command) { + const char* rsh = (rsh_command && *rsh_command) ? rsh_command : "ssh"; + + /* Whitespace-split the remote-shell command into the leading argv words so + * "-e 'ssh -p 2222'" (or "--rsh=ssh -p 2222") works like rsync's rsh. A + * blank command falls back to the default "ssh". */ + char* copy = str_dup(rsh); + if (!copy) + return NULL; + char* save = NULL; + int nwords = 0; + char** words = NULL; + for (char* tok = strtok_r(copy, " \t", &save); tok; tok = strtok_r(NULL, " \t", &save)) { + char** grown = realloc(words, (size_t)(nwords + 1) * sizeof(char*)); + if (!grown) { + for (int i = 0; i < nwords; i++) + free(words[i]); + free(words); + free(copy); + return NULL; + } + words = grown; + words[nwords] = str_dup(tok); + if (!words[nwords]) { + for (int i = 0; i < nwords; i++) + free(words[i]); + free(words); + free(copy); + return NULL; + } + nwords++; + } + free(copy); + if (nwords == 0) { + words = malloc(sizeof(char*)); + if (!words) + return NULL; + words[0] = str_dup("ssh"); + if (!words[0]) { + free(words); + return NULL; + } + nwords = 1; + } + + /* Fixed tail: three -o pairs (6) + optional -p/value (2) + user@host + + * remote command + terminating NULL. */ + int port_extra = (port > 0 && port != 22) ? 2 : 0; + size_t total = (size_t)nwords + 6 + (size_t)port_extra + 3; + char** argv = calloc(total, sizeof(char*)); + if (!argv) { + for (int i = 0; i < nwords; i++) + free(words[i]); + free(words); + return NULL; + } + int ac = 0; + for (int i = 0; i < nwords; i++) + argv[ac++] = words[i]; + free(words); + + char* tail[] = {"-o", "Compression=no", + "-o", "ControlMaster=auto", + "-o", "ControlPath=~/.cache/fastsync-%r@%h:%p"}; + for (size_t i = 0; i < sizeof(tail) / sizeof(tail[0]); i++) + argv[ac++] = str_dup(tail[i]); + if (port_extra) { + char port_str[16]; + snprintf(port_str, sizeof(port_str), "%d", port); + argv[ac++] = str_dup("-p"); + argv[ac++] = str_dup(port_str); + } + argv[ac++] = str_dup(userhost); + argv[ac++] = str_dup(remote_command); + argv[ac] = NULL; + return argv; +} + +void ssh_free_client_argv(char** argv) { + if (!argv) + return; + for (int i = 0; argv[i]; i++) + free(argv[i]); + free(argv); +} + Client* client_connect_ssh(const char* destination, int port, const char* server_path, - bool old_args) { + bool old_args, const char* rsh_command, bool blocking_io) { RemoteDest r; if (parse_remote_dest(destination, &r) != 0) { char* escaped = output_escape(destination, false); @@ -142,6 +234,17 @@ Client* client_connect_ssh(const char* destination, int port, const char* server setsockopt(sv[1], SOL_SOCKET, SO_SNDBUF, &buf_size, sizeof(buf_size)); setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &buf_size, sizeof(buf_size)); + /* By default the SSH transport socket gets the same read/write timeout as + * the TCP transport so a wedged remote shell cannot hang forever. With + * --blocking-io the timeouts are skipped and the socket blocks naturally. */ + if (!blocking_io) { + struct timeval tv; + tv.tv_sec = tcp_get_timeout_sec(); + tv.tv_usec = 0; + setsockopt(sv[0], SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); + setsockopt(sv[0], SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)); + } + int exec_pipe[2]; if (pipe(exec_pipe) < 0) { log_perror("pipe failed"); @@ -187,29 +290,17 @@ Client* client_connect_ssh(const char* destination, int port, const char* server else snprintf(ssh_user, ssh_user_len, "%s", r.host); - char* ssh_argv[16]; - int ac = 0; - char port_str[16]; char* remote_command = ssh_build_remote_command(server_path, old_args); if (!remote_command) ssh_child_setup_failed(exec_pipe[1]); - ssh_argv[ac++] = "ssh"; - ssh_argv[ac++] = "-o"; - ssh_argv[ac++] = "Compression=no"; - ssh_argv[ac++] = "-o"; - ssh_argv[ac++] = "ControlMaster=auto"; - ssh_argv[ac++] = "-o"; - ssh_argv[ac++] = "ControlPath=~/.cache/fastsync-%r@%h:%p"; - if (port > 0 && port != 22) { - ssh_argv[ac++] = "-p"; - snprintf(port_str, sizeof(port_str), "%d", port); - ssh_argv[ac++] = port_str; - } - ssh_argv[ac++] = ssh_user; - ssh_argv[ac++] = remote_command; - ssh_argv[ac] = NULL; - execvp("ssh", ssh_argv); - log_perror("exec of ssh failed"); + char** ssh_argv = ssh_build_client_argv(rsh_command, port, ssh_user, remote_command); + free(ssh_user); + free(remote_command); + if (!ssh_argv) + ssh_child_setup_failed(exec_pipe[1]); + execvp(ssh_argv[0], ssh_argv); + log_perror("exec of remote shell failed"); + ssh_free_client_argv(ssh_argv); ssh_child_setup_failed(exec_pipe[1]); } diff --git a/src/shared/transport_ssh.h b/src/shared/transport_ssh.h index e46c687..89b7b08 100644 --- a/src/shared/transport_ssh.h +++ b/src/shared/transport_ssh.h @@ -4,7 +4,15 @@ #include "transport_tcp.h" Client* client_connect_ssh(const char* destination, int port, const char* server_path, - bool old_args); + bool old_args, const char* rsh_command, bool blocking_io); char* ssh_build_remote_command(const char* server_path, bool old_args); +/* Build the NULL-terminated child argv for the remote-shell client (argv[0] is + * the exec/execvp program). rsh_command is whitespace-split into leading argv + * words (NULL or "" selects the default "ssh"); the standard -o family, the + * optional -p port, the user@host and the remote command are appended. Every + * string (including argv[0]) is heap-owned; free with ssh_free_client_argv. */ +char** ssh_build_client_argv(const char* rsh_command, int port, const char* userhost, + const char* remote_command); +void ssh_free_client_argv(char** argv); #endif diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index fb853f0..e7a7365 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -4604,3 +4604,53 @@ class TestExtendedAttributes: fields = record.split(":") assert len(fields) == 5 assert fields[0] == str(uid), f"reserved uid field {fields[0]} != source uid {uid}" + + +class TestConnectivityClientOptions: + """Phase 5 connectivity launch options (--outbuf, --blocking-io). + + These are client-side launch concerns: --outbuf only restyles stdout/stderr + buffering and --blocking-io only skips the SSH transport socket timeouts. + Over the TCP transport both must parse cleanly and be inert -- a transfer + must still complete and verify byte-for-byte.""" + + def _source_and_dest(self, name): + source = os.path.join(TEST_DATA_DIR, name + "_src") + dest = os.path.join(TEST_DATA_DIR, name + "_dst") + clean_dir(source) + clean_dir(dest) + return source, dest + + @pytest.mark.parametrize("flag", ["--outbuf=N", "--outbuf=L", "--outbuf=B", + "--blocking-io"]) + def test_option_does_not_break_transfer(self, shared_server, flag): + source, dest = self._source_and_dest("connopt") + with open(os.path.join(source, "hello.txt"), "wb") as f: + f.write(b"connectivity options\n" * 100) + with open(os.path.join(source, "data.bin"), "wb") as f: + f.write(os.urandom(512 * 1024)) + + result, _ = run_client(source, dest, flags=[flag], port=shared_server.port) + assert result.returncode == 0, \ + f"{flag} failed: {(result.stderr or result.stdout)[:300]}" + mismatches, missing = verify_transfer(source, get_dest_received_dir(dest, source)) + assert not mismatches and not missing, \ + f"{flag}: mismatches={mismatches[:3]} missing={missing[:3]}" + + def test_rejects_invalid_outbuf(self, shared_server): + source, dest = self._source_and_dest("connopt_bad") + with open(os.path.join(source, "x.txt"), "wb") as f: + f.write(b"x") + result, _ = run_client(source, dest, flags=["--outbuf=Z"], port=shared_server.port) + assert result.returncode != 0, "--outbuf=Z must be rejected" + + def test_blocking_io_does_not_break_compressed_transfer(self, shared_server): + source, dest = self._source_and_dest("connopt_zlib") + with open(os.path.join(source, "text.txt"), "wb") as f: + f.write(b"compress me\n" * 4096) + result, _ = run_client(source, dest, flags=["--blocking-io", "-c"], + port=shared_server.port) + assert result.returncode == 0, \ + f"--blocking-io -c failed: {(result.stderr or result.stdout)[:300]}" + mismatches, missing = verify_transfer(source, get_dest_received_dir(dest, source)) + assert not mismatches and not missing diff --git a/tests/integration/test_ssh.py b/tests/integration/test_ssh.py index 931dbfe..173411b 100644 --- a/tests/integration/test_ssh.py +++ b/tests/integration/test_ssh.py @@ -64,11 +64,12 @@ def setup_test_data(): shutil.rmtree(DEST_DIR, ignore_errors=True) -def _run_ssh_test(name, flags, expected_missing=None): +def _run_ssh_test(name, flags, expected_missing=None, path_args=None): ssh_dest = f"localhost:{DEST_DIR}" clean_dir(DEST_DIR) - cmd = CLIENT_CMD + [SOURCE_DIR, ssh_dest, "--save-to-disk", - "--fastsync-server-path", os.path.join(BUILD_DIR, "server")] + flags + if not path_args: + path_args = ["--fastsync-server-path", os.path.join(BUILD_DIR, "server")] + cmd = CLIENT_CMD + [SOURCE_DIR, ssh_dest, "--save-to-disk"] + path_args + flags start = __import__("time").monotonic() result = subprocess.run(cmd, text=True, capture_output=True) duration = __import__("time").monotonic() - start @@ -142,3 +143,41 @@ class TestSSHFeatures: def test_preallocate(self): r = _run_ssh_test("SSH Preallocate (--preallocate)", ["--preallocate"]) assert r["status"] == "Success", r["error"] + + +class TestSSHConnectivity: + """Phase 5 connectivity options: -e/--rsh, --rsync-path, --blocking-io, + --outbuf. These are client-side launch concerns, so each must parse and + still drive a real SSH transfer to completion.""" + + @pytest.fixture(autouse=True) + def require_ssh(self): + if not SSH_AVAILABLE: + pytest.skip(SSH_SKIP_REASON) + + def test_rsh_short_form_selects_ssh(self): + r = _run_ssh_test("SSH -e ssh", ["-e", "ssh"]) + assert r["status"] == "Success", r["error"] + + def test_rsh_long_form_selects_ssh(self): + r = _run_ssh_test("SSH --rsh=ssh", ["--rsh=ssh"]) + assert r["status"] == "Success", r["error"] + + def test_rsync_path_aliases_server_path(self): + r = _run_ssh_test("SSH --rsync-path", + [], + path_args=["--rsync-path", os.path.join(BUILD_DIR, "server")]) + assert r["status"] == "Success", r["error"] + + def test_blocking_io(self): + r = _run_ssh_test("SSH --blocking-io", ["--blocking-io"]) + assert r["status"] == "Success", r["error"] + + @pytest.mark.parametrize("mode", ["N", "L", "B"]) + def test_outbuf_mode(self, mode): + r = _run_ssh_test(f"SSH --outbuf={mode}", [f"--outbuf={mode}"]) + assert r["status"] == "Success", r["error"] + + def test_blocking_io_with_compression(self): + r = _run_ssh_test("SSH --blocking-io -c", ["--blocking-io", "-c"]) + assert r["status"] == "Success", r["error"] diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 758c528..6d999ad 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -831,9 +831,6 @@ static void test_parse_args_rejects_unimplemented_options() { "--delete-excluded", "--max-delete", "--prune-empty-dirs", - "-e", - "--rsh", - "--rsync-path", "--address", "--bind-address", "--ipv6", @@ -1210,6 +1207,117 @@ static void test_parse_args_old_args() { config_delete(cfg); } +/* Phase 5 connectivity: -e/--rsh select the remote-shell program. Both the + * short (space-separated value) and long (=value and space) forms parse, and + * a multi-word command line is preserved verbatim for the transport layer. */ +static void test_parse_args_rsh() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "-e", "ssh -p 2222", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), 0); + EXPECT_EQ_STR(cfg->rsh_command, "ssh -p 2222"); + config_delete(cfg); + + cfg = config_create(); + positional_count = 0; + char* argv_eq[] = {"fastsync", "--rsh=customsh", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_eq, positional_args, &positional_count), 0); + EXPECT_EQ_STR(cfg->rsh_command, "customsh"); + config_delete(cfg); + + cfg = config_create(); + positional_count = 0; + char* argv_space[] = {"fastsync", "--rsh", "ssh -l bob", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 5, argv_space, positional_args, &positional_count), 0); + EXPECT_EQ_STR(cfg->rsh_command, "ssh -l bob"); + config_delete(cfg); + + /* A missing value is a hard error. */ + cfg = config_create(); + positional_count = 0; + char* argv_missing[] = {"fastsync", "-e"}; + EXPECT_EQ_INT(parse_args(cfg, 2, argv_missing, positional_args, &positional_count), -1); + config_delete(cfg); +} + +/* --rsync-path is rsync's spelling for the server program path: it aliases + * fastsync_server_path exactly like --fastsync-server-path. */ +static void test_parse_args_rsync_path_alias() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "--rsync-path", "/usr/bin/fastsync-server", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), 0); + EXPECT_EQ_STR(cfg->fastsync_server_path, "/usr/bin/fastsync-server"); + config_delete(cfg); + + cfg = config_create(); + positional_count = 0; + char* argv_eq[] = {"fastsync", "--rsync-path=/opt/bin/srv", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_eq, positional_args, &positional_count), 0); + EXPECT_EQ_STR(cfg->fastsync_server_path, "/opt/bin/srv"); + config_delete(cfg); +} + +/* --blocking-io is a plain boolean flag that leaves the SSH socket with no + * timeouts; the default is off. */ +static void test_parse_args_blocking_io() { + Config* cfg = config_create(); + EXPECT_FALSE(cfg->blocking_io); + char* argv[] = {"fastsync", "--blocking-io", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), 0); + EXPECT_TRUE(cfg->blocking_io); + config_delete(cfg); +} + +/* --outbuf=N|L|B maps onto the OUTBUF_* modes (default: block). Garbage is + * rejected, never silently coerced. */ +static void test_parse_args_outbuf() { + Config* cfg = config_create(); + EXPECT_EQ_INT(cfg->outbuf, OUTBUF_BLOCK); + int positional_args[2]; + int positional_count = 0; + + char* argv_n[] = {"fastsync", "--outbuf=N", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_n, positional_args, &positional_count), 0); + EXPECT_EQ_INT(cfg->outbuf, OUTBUF_NONE); + config_delete(cfg); + + cfg = config_create(); + positional_count = 0; + char* argv_l[] = {"fastsync", "--outbuf", "L", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 5, argv_l, positional_args, &positional_count), 0); + EXPECT_EQ_INT(cfg->outbuf, OUTBUF_LINE); + config_delete(cfg); + + cfg = config_create(); + positional_count = 0; + char* argv_b[] = {"fastsync", "--outbuf=b", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_b, positional_args, &positional_count), 0); + EXPECT_EQ_INT(cfg->outbuf, OUTBUF_BLOCK); + config_delete(cfg); + + static const char* const bad[] = {"G", "X", ""}; + for (size_t i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) { + cfg = config_create(); + positional_count = 0; + char option[32]; + snprintf(option, sizeof(option), "--outbuf=%s", bad[i]); + char* argv_bad[] = {"fastsync", option, "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_bad, positional_args, &positional_count), -1); + config_delete(cfg); + } + + cfg = config_create(); + positional_count = 0; + char* argv_missing[] = {"fastsync", "--outbuf"}; + EXPECT_EQ_INT(parse_args(cfg, 2, argv_missing, positional_args, &positional_count), -1); + config_delete(cfg); +} + static void test_parse_args_fsync() { Config* cfg = config_create(); char* argv[] = {"fastsync", "--fsync", "/src", "/dst"}; @@ -2556,6 +2664,10 @@ void test_client_cli() { test_parse_args_no_preserve_blocks_implicit_metadata(); test_parse_args_rejects_unsafe_negation(); test_parse_args_old_args(); + test_parse_args_rsh(); + test_parse_args_rsync_path_alias(); + test_parse_args_blocking_io(); + test_parse_args_outbuf(); test_parse_args_fsync(); test_parse_args_existing(); test_parse_args_ignore_times(); diff --git a/tests/test_transport_ssh.c b/tests/test_transport_ssh.c index 7a2f3d6..fe3b29f 100644 --- a/tests/test_transport_ssh.c +++ b/tests/test_transport_ssh.c @@ -4,13 +4,13 @@ static void test_ssh_connect_invalid_dest_no_colon() { /* cppcheck-suppress constVariablePointer */ - Client* client = client_connect_ssh("invalid-destination-no-colon", 22, NULL, false); + Client* client = client_connect_ssh("invalid-destination-no-colon", 22, NULL, false, NULL, false); EXPECT_NULL(client); } static void test_ssh_connect_invalid_dest_empty() { /* cppcheck-suppress constVariablePointer */ - Client* client = client_connect_ssh("", 22, NULL, false); + Client* client = client_connect_ssh("", 22, NULL, false, NULL, false); EXPECT_NULL(client); } @@ -21,7 +21,7 @@ static void test_ssh_connect_malformed() { setenv("PATH", "", 1); /* cppcheck-suppress constVariablePointer */ - Client* client = client_connect_ssh(":", 22, NULL, false); + Client* client = client_connect_ssh(":", 22, NULL, false, NULL, false); if (saved_path) { setenv("PATH", saved_path, 1); @@ -36,7 +36,8 @@ static void test_ssh_connect_malformed() { /* Test client_connect_ssh with valid format but unreachable host. * The function launches ssh which will fail to connect, returns a Client. */ static void test_ssh_connect_unreachable() { - Client* client = client_connect_ssh("nonexistent.invalid:/remote/path", 22, NULL, false); + Client* client = + client_connect_ssh("nonexistent.invalid:/remote/path", 22, NULL, false, NULL, false); if (client != NULL) { client_disconnect(client); client_delete(client); @@ -58,10 +59,58 @@ static void test_ssh_remote_command_argument_modes() { free(command); } +/* The build for a single-word argv is [prog, six -o args, user, command]. */ + +static void test_ssh_build_client_argv_default_is_ssh() { + char** argv = ssh_build_client_argv(NULL, 0, "u@h", "'srv' --stdio"); + EXPECT_NOT_NULL(argv); + EXPECT_EQ_STR(argv[0], "ssh"); + EXPECT_EQ_STR(argv[1], "-o"); + EXPECT_EQ_STR(argv[7], "u@h"); + EXPECT_EQ_STR(argv[8], "'srv' --stdio"); + EXPECT_NULL(argv[9]); + ssh_free_client_argv(argv); +} + +/* A configured rsh must replace "ssh" as argv[0] (and never leak the default). */ +static void test_ssh_build_client_argv_uses_custom_rsh() { + char** argv = ssh_build_client_argv("myrsh", 0, "u@h", "rc"); + EXPECT_NOT_NULL(argv); + EXPECT_EQ_STR(argv[0], "myrsh"); + EXPECT_NULL(argv[9]); + ssh_free_client_argv(argv); +} + +/* A multi-word rsh command line (rsync -e "ssh -p 2222") is split into the + * leading argv words; a non-default port adds a -p/value pair. */ +static void test_ssh_build_client_argv_whitespace_command_and_port() { + char** argv = ssh_build_client_argv("ssh -p 2222", 0, "u@h", "rc"); + EXPECT_NOT_NULL(argv); + EXPECT_EQ_STR(argv[0], "ssh"); + EXPECT_EQ_STR(argv[1], "-p"); + EXPECT_EQ_STR(argv[2], "2222"); + EXPECT_NULL(argv[11]); + ssh_free_client_argv(argv); + + argv = ssh_build_client_argv("ssh", 2222, "u@h", "rc"); + EXPECT_NOT_NULL(argv); + EXPECT_EQ_STR(argv[0], "ssh"); + /* Flat [prog, -o x6, -p, port, user, command]. */ + EXPECT_EQ_STR(argv[7], "-p"); + EXPECT_EQ_STR(argv[8], "2222"); + EXPECT_EQ_STR(argv[9], "u@h"); + EXPECT_EQ_STR(argv[10], "rc"); + EXPECT_NULL(argv[11]); + ssh_free_client_argv(argv); +} + void test_transport_ssh() { test_ssh_connect_invalid_dest_no_colon(); test_ssh_connect_invalid_dest_empty(); test_ssh_connect_malformed(); test_ssh_connect_unreachable(); test_ssh_remote_command_argument_modes(); + test_ssh_build_client_argv_default_is_ssh(); + test_ssh_build_client_argv_uses_custom_rsh(); + test_ssh_build_client_argv_whitespace_command_and_port(); } From cdcaf21acd08a8dd1b3806e0f4e9de5eaf0184b7 Mon Sep 17 00:00:00 2001 From: TapTap Date: Wed, 9 Sep 2026 14:23:59 +0200 Subject: [PATCH 2/2] fix(p5-rsh): NULL-check argv tail str_dups in ssh_build_client_argv --- src/shared/transport_ssh.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index fe3c18e..f3c7709 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -189,18 +189,40 @@ char** ssh_build_client_argv(const char* rsh_command, int port, const char* user char* tail[] = {"-o", "Compression=no", "-o", "ControlMaster=auto", "-o", "ControlPath=~/.cache/fastsync-%r@%h:%p"}; - for (size_t i = 0; i < sizeof(tail) / sizeof(tail[0]); i++) - argv[ac++] = str_dup(tail[i]); + for (size_t i = 0; i < sizeof(tail) / sizeof(tail[0]); i++) { + argv[ac] = str_dup(tail[i]); + if (!argv[ac]) + goto fail_argv; + ac++; + } if (port_extra) { char port_str[16]; snprintf(port_str, sizeof(port_str), "%d", port); - argv[ac++] = str_dup("-p"); - argv[ac++] = str_dup(port_str); + argv[ac] = str_dup("-p"); + if (!argv[ac]) + goto fail_argv; + ac++; + argv[ac] = str_dup(port_str); + if (!argv[ac]) + goto fail_argv; + ac++; } - argv[ac++] = str_dup(userhost); - argv[ac++] = str_dup(remote_command); + argv[ac] = str_dup(userhost); + if (!argv[ac]) + goto fail_argv; + ac++; + argv[ac] = str_dup(remote_command); + if (!argv[ac]) + goto fail_argv; + ac++; argv[ac] = NULL; return argv; + +fail_argv: + for (int i = 0; i < ac; i++) + free(argv[i]); + free(argv); + return NULL; } void ssh_free_client_argv(char** argv) {