diff --git a/src/client/client_validation.c b/src/client/client_validation.c index 2ff7868..eb71c3c 100644 --- a/src/client/client_validation.c +++ b/src/client/client_validation.c @@ -75,6 +75,13 @@ bool validate_config(const Config* config) { log_message(LOG_LEVEL_ERROR, "-4/--ipv4 and -6/--ipv6 are mutually exclusive"); return false; } + /* rsync 3.4.1 rejects --inplace together with --partial-dir (exit 1): the + inplace write path bypasses partial staging, so a partial-dir name would be + silently ignored. Match rsync's message and refuse before any I/O. */ + if (config->inplace && config->partial_dir) { + log_message(LOG_LEVEL_ERROR, "--inplace cannot be used with --partial-dir"); + return false; + } if (config->log_file_format && !config->log_file) { log_message(LOG_LEVEL_ERROR, "--log-file-format requires --log-file"); return false; diff --git a/src/shared/compression.c b/src/shared/compression.c index c7bba22..a3d7d66 100644 --- a/src/shared/compression.c +++ b/src/shared/compression.c @@ -644,8 +644,7 @@ static Data* zstd_decompress(Data* compressed_data, size_t maximum_size) { } if (ret > 0 && output.pos == output.size) { if (buf_size >= hard_limit || buf_size > SIZE_MAX / 2) { - log_message(LOG_LEVEL_ERROR, "Decompressed data exceeds %llu bytes", - (unsigned long long)MAX_DECOMPRESSED_SIZE); + log_message(LOG_LEVEL_ERROR, "Decompressed data exceeds %llu bytes", hard_limit); data_destroy(uncompressed_data); uncompressed_data = NULL; goto cleanup; diff --git a/src/shared/config.c b/src/shared/config.c index ba3f34d..f4b255d 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -234,7 +234,7 @@ Config* config_create(void) { * server_host NULL and would crash later consumers, so fail the whole create * (every caller already handles a NULL return). */ if (!config->server_host) { - free(config); + config_delete(config); return NULL; } return config; diff --git a/tests/integration/test_daemon.py b/tests/integration/test_daemon.py index 0212616..ae1d765 100644 --- a/tests/integration/test_daemon.py +++ b/tests/integration/test_daemon.py @@ -63,6 +63,10 @@ DETACH_MODULE = os.path.join(MODULE_ROOT, "detach") DETACH_CONF = os.path.join(TEST_DATA_DIR, "fastsyncd_detach.conf") DETACH_PORT = None +# A dedicated config for the umask test: the daemon must be launched in the real +# (double-fork) detach path, whose daemonize() applies umask(022). +UMASK_CONF = os.path.join(TEST_DATA_DIR, "fastsyncd_umask.conf") + # Passwords are never sent as plaintext and never logged; these literals are # only hashed into the server credential file / client password file. ALICE_PASS = "alice-s3cret" @@ -332,20 +336,43 @@ class TestDaemonModuleSelection: assert not missing, f"missing: {missing[:5]}" assert not mismatches, f"mismatch: {mismatches[:5]}" - def test_daemon_new_dirs_not_world_writable(self, daemon): + def test_daemon_new_dirs_not_world_writable(self): """The daemon must not force umask 0: implied parent directories created - without -p are the source default (0755 under a 022 umask), never - world-writable 0777.""" + without -p are the source default (0755 under the daemon's 022 umask), + never world-writable 0777. + + This drives the real double-fork detach path, where the umask(022) fix + lives (daemonize()); the --no-detach path never calls it. The launcher + is run with umask 0, so without the fix the daemon would inherit 0 and + create a 0777 directory; with the fix the assertion below fails only if + the fix regresses.""" + port = _find_free_port() + with open(UMASK_CONF, "w") as f: + f.write("port = %d\n\n[files]\npath = %s\n" % (port, FILES_MODULE)) sub = os.path.join(FILES_MODULE, "umask_check") shutil.rmtree(sub, ignore_errors=True) os.makedirs(sub, exist_ok=True) - result = _push("127.0.0.1::files/umask_check", daemon.port) - assert result.returncode == 0, result.stderr or result.stdout - received = get_dest_received_dir(sub, SOURCE_DIR) - nested = os.path.join(received, "nested") - assert os.path.isdir(nested), f"nested dir missing under {received}" - mode = stat.S_IMODE(os.stat(nested).st_mode) - assert (mode & 0o022) == 0, f"implied directory is group/other writable: {oct(mode)}" + log_path = os.path.join(TEST_DATA_DIR, "fastsyncd_umask.log") + log = open(log_path, "w") + cmd = SERVER_CMD + ["--daemon", "--config", UMASK_CONF, "--allow-unauthenticated"] + proc = subprocess.Popen(cmd, stdout=log, stderr=log, stdin=subprocess.DEVNULL, + preexec_fn=lambda: os.umask(0)) + try: + _wait_for_port(port, timeout=15) + result = _push("127.0.0.1::files/umask_check", port) + assert result.returncode == 0, result.stderr or result.stdout + received = get_dest_received_dir(sub, SOURCE_DIR) + nested = os.path.join(received, "nested") + assert os.path.isdir(nested), f"nested dir missing under {received}" + mode = stat.S_IMODE(os.stat(nested).st_mode) + assert (mode & 0o022) == 0, f"implied directory is group/other writable: {oct(mode)}" + finally: + _kill_by_cmdline_marker(UMASK_CONF) + log.close() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() class TestDaemonRejection: diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index b8b2ff9..d86f8f6 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -578,13 +578,18 @@ static void test_parse_args_partial_dir_implies_partial() { config_delete(cfg); } { - /* --inplace bypasses partial staging: the implication must not fire. */ + /* --inplace bypasses partial staging, so parse_args must not set the + implied --partial; the combination itself is invalid (rsync parity: + "--inplace cannot be used with --partial-dir"), so validation rejects. */ Config* cfg = config_create(); char* argv[] = {"fastsync", "--inplace", "--partial-dir=.partial", "/src", "/dst"}; int positional_args[2]; int positional_count = 0; EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), 0); EXPECT_FALSE(cfg->partial); + cfg->send_directory = str_dup("/src"); + cfg->receive_root_directory = str_dup("/dst"); + EXPECT_FALSE(validate_config(cfg)); config_delete(cfg); } { diff --git a/tests/test_file.c b/tests/test_file.c index d0124de..69912f0 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -391,52 +391,82 @@ static void test_file_open_temp_dir_symlink_confinement() { rmdir("test_tempdir_link_root/scratch"); rmdir(root); rmdir(outside); - EXPECT_EQ_INT(mkdir(root, 0755), 0); - EXPECT_EQ_INT(mkdir(outside, 0755), 0); - EXPECT_NOT_NULL(realpath(root, root_abs)); - EXPECT_NOT_NULL(realpath(outside, outside_abs)); - int root_fd = open(root_abs, O_RDONLY | O_DIRECTORY | O_CLOEXEC); - EXPECT_TRUE(root_fd >= 0); - // cppcheck-suppress knownConditionTrueFalse - if (root_fd < 0) { - rmdir(root); - rmdir(outside); - return; + + int mkdir_root_ret = mkdir(root, 0755); + int mkdir_outside_ret = mkdir(outside, 0755); + bool root_resolved = realpath(root, root_abs) != NULL; + bool outside_resolved = realpath(outside, outside_abs) != NULL; + int root_fd = root_resolved ? open(root_abs, O_RDONLY | O_DIRECTORY | O_CLOEXEC) : -1; + + bool root_set = false; + bool scratch_ok = false; + int scratch_fd = -1; + bool escape_staged = false; + int escape_fd = 0; + bool inside_staged = false; + int inside_fd = -1; + char* scratch = NULL; + char* escape = NULL; + char* inside_link = NULL; + + /* Only touch the global authorized root and the scratch fixtures once the + setup succeeded; the teardown below always runs regardless. */ + if (root_fd >= 0 && outside_resolved) { + root_set = utils_set_authorized_root(root_fd, root_abs); + + /* An existing in-root scratch dir opens normally. */ + scratch = path_cat(root_abs, "scratch"); + if (scratch && mkdir(scratch, 0755) == 0) { + scratch_ok = true; + scratch_fd = file_open_temp_dir(scratch); + if (scratch_fd >= 0) + close(scratch_fd); + } + + /* A symlink whose target is outside the root is refused. */ + escape = path_cat(root_abs, "escape"); + if (escape && symlink(outside_abs, escape) == 0) { + escape_staged = true; + escape_fd = file_open_temp_dir(escape); + } + + /* A symlink that stays inside the root is accepted (EXDEV fallback). */ + inside_link = path_cat(root_abs, "inside_link"); + if (inside_link && scratch && symlink(scratch, inside_link) == 0) { + inside_staged = true; + inside_fd = file_open_temp_dir(inside_link); + if (inside_fd >= 0) + close(inside_fd); + } } - EXPECT_TRUE(utils_set_authorized_root(root_fd, root_abs)); - - /* An existing in-root scratch dir opens normally. */ - char* scratch = path_cat(root_abs, "scratch"); - EXPECT_NOT_NULL(scratch); - EXPECT_EQ_INT(mkdir(scratch, 0755), 0); - int scratch_fd = file_open_temp_dir(scratch); - EXPECT_TRUE(scratch_fd >= 0); - close(scratch_fd); - - /* A symlink whose target is outside the root is refused. */ - char* escape = path_cat(root_abs, "escape"); - EXPECT_NOT_NULL(escape); - EXPECT_EQ_INT(symlink(outside_abs, escape), 0); - EXPECT_EQ_INT(file_open_temp_dir(escape), -1); - - /* A symlink that stays inside the root is accepted (EXDEV fallback path). */ - char* inside_link = path_cat(root_abs, "inside_link"); - EXPECT_NOT_NULL(inside_link); - EXPECT_EQ_INT(symlink(scratch, inside_link), 0); - int link_fd = file_open_temp_dir(inside_link); - EXPECT_TRUE(link_fd >= 0); - close(link_fd); + /* Release the global authorized root and all fixtures BEFORE asserting: + EXPECT_* returns early on failure, so a failed assertion must not be able + to leave the process state poisoned or leak root_fd. */ + utils_set_authorized_root(-1, NULL); + if (root_fd >= 0) + close(root_fd); free(inside_link); free(escape); free(scratch); - utils_set_authorized_root(-1, NULL); - close(root_fd); unlink("test_tempdir_link_root/escape"); unlink("test_tempdir_link_root/inside_link"); rmdir("test_tempdir_link_root/scratch"); rmdir(root); rmdir(outside); + + EXPECT_EQ_INT(mkdir_root_ret, 0); + EXPECT_EQ_INT(mkdir_outside_ret, 0); + EXPECT_TRUE(root_resolved); + EXPECT_TRUE(outside_resolved); + EXPECT_TRUE(root_fd >= 0); + EXPECT_TRUE(root_set); + EXPECT_TRUE(scratch_ok); + EXPECT_TRUE(scratch_fd >= 0); + EXPECT_TRUE(escape_staged); + EXPECT_EQ_INT(escape_fd, -1); + EXPECT_TRUE(inside_staged); + EXPECT_TRUE(inside_fd >= 0); } /* Issue #251: file_save_to_disk_full must distinguish receiver-side skips diff --git a/tests/test_protocol.c b/tests/test_protocol.c index 2665096..483ee82 100644 --- a/tests/test_protocol.c +++ b/tests/test_protocol.c @@ -741,7 +741,7 @@ static void test_protocol_throttle_bytes_unlimited() { clock_gettime(CLOCK_MONOTONIC, &now); long long elapsed_ms = (now.tv_sec - start.tv_sec) * 1000LL + (now.tv_nsec - start.tv_nsec) / 1000000LL; - EXPECT_TRUE(elapsed_ms < 50); + EXPECT_TRUE(elapsed_ms < 2000); protocol_session_unbind(); }