diff --git a/src/client/scanner.c b/src/client/scanner.c index 3a8722d..3266c0a 100644 --- a/src/client/scanner.c +++ b/src/client/scanner.c @@ -179,7 +179,7 @@ static bool entry_passes_selection(const FileListSet* file_list, const FilterRul static void scanner_capture_xattrs(const DirectoryScanner* scanner, File* file) { if (!scanner || !file || !(scanner->options.preserve_xattrs || scanner->options.preserve_acls)) return; - file->xattrs = xattr_capture_path(file->path); + file->xattrs = xattr_capture_path(file->path, scanner->options.preserve_acls); } /* Apply --hard-links (-H) detection to one regular File. On a sibling (a @@ -1434,7 +1434,7 @@ static void scan_root_entry(const ScannerOptions* options, const FilterNode* roo } if ((options->preserve_xattrs || options->preserve_acls) && !(file->link_group != 0 && !file->link_first)) - file->xattrs = xattr_capture_path(file->path); + file->xattrs = xattr_capture_path(file->path, options->preserve_acls); if (!array_list_add(root_files, file)) { free(rel); file_destroy(file); diff --git a/src/shared/file.c b/src/shared/file.c index 47b13ab..6d4b7b9 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -1248,11 +1248,19 @@ static bool file_to_disk_secure_link_impl(const char* path, const char* basis_pa if (linked) { int target_dirfd = scratch_dirfd >= 0 ? scratch_dirfd : dirfd; if (use_fsync) { - int tfd = openat(target_dirfd, tmp, O_RDONLY | O_NOFOLLOW | O_CLOEXEC); - if (tfd < 0 || fsync(tfd) != 0) { + /* O_NONBLOCK: the freshly linked temp is normally the basis's regular + file, but a raced-in FIFO at the name must not block this reopen + forever. With O_NONBLOCK such an open fails with ENXIO instead of + blocking, which is treated as a benign fsync-skip (the link itself + is still installed); any other open/fsync failure falls back to the + byte-copy path as before. */ + int tfd = openat(target_dirfd, tmp, O_RDONLY | O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK); + if (tfd < 0) { + if (errno != ENXIO) + linked = false; + } else if (fsync(tfd) != 0) { linked = false; - if (tfd >= 0) - close(tfd); + close(tfd); } else { close(tfd); } diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index 8e3c2c7..4ead296 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -263,7 +263,8 @@ static FileSaveResult file_save_hardlink_sibling(const char* root_directory, con free(destination_path); return absent_result; } - FileXattrList* sibling_xattrs = cfg->use_xattrs ? xattr_capture_path(staged_first) : NULL; + FileXattrList* sibling_xattrs = + cfg->use_xattrs ? xattr_capture_path(staged_first, cfg->preserve_acls) : NULL; bool ok = file_to_disk_secure_link_attrs( staged_sibling, staged_first, content, content_size, preallocate, file->metadata, preserve_executability, use_fsync, sibling_xattrs, cfg ? cfg->fake_super : false, NULL); @@ -295,7 +296,8 @@ static FileSaveResult file_save_hardlink_sibling(const char* root_directory, con return absent_result; } const char* temp_dir = (cfg && cfg->temp_dir) ? cfg->temp_dir : NULL; - FileXattrList* sibling_xattrs = cfg->use_xattrs ? xattr_capture_path(first_disk) : NULL; + FileXattrList* sibling_xattrs = + cfg->use_xattrs ? xattr_capture_path(first_disk, cfg->preserve_acls) : NULL; bool ok = file_to_disk_secure_link_attrs( destination_path, first_disk, content, content_size, preallocate, file->metadata, preserve_executability, use_fsync, sibling_xattrs, cfg ? cfg->fake_super : false, temp_dir); @@ -1276,14 +1278,27 @@ static bool basis_quick_matches(const Config* config, const struct stat* st, tim /* Search the basis-dir list in command-line order and return the first exact match. When load_content is true the matched bytes are kept in out->content - so the caller can materialize the file without re-reading it. */ + so the caller can materialize the file without re-reading it. + + An exact match ALSO requires the basis bytes' digest to equal the source's, + so `hash_content` gates the content read/hash itself. A server-contacting + --dry-run passes hash_content=false: no basis file may be read or hashed + (that would be a 1-bit content oracle against a client-supplied digest), so a + metadata-only pass can never confirm a hit and declines it. The real path + always passes hash_content=true, keeping its behavior byte-for-byte. */ static bool basis_match_find(const Config* config, const char* check_path, unsigned long long check_size, time_t check_mtime, long check_mtime_nsec, const uint8_t* check_digest, - size_t check_digest_len, bool load_content, BasisMatch* out) { + size_t check_digest_len, bool load_content, bool hash_content, + BasisMatch* out) { memset(out, 0, sizeof(*out)); if (!config || !config_has_basis(config) || config->ignore_times) return false; + /* Dry-run: never read/hash basis content. A hit cannot be decided from + metadata alone, so report no match (the caller treats it as would-transfer) + without touching the file's contents. */ + if (!hash_content) + return false; for (int i = 0; i < config->basis_count; i++) { const BasisDest* entry = &config->basis_dirs[i]; char* basis_dir = path_cat(config->receive_root_directory, entry->path); @@ -1911,10 +1926,13 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat When dry_run is set and the file is not already up to date the receiver must materialize nothing (no basis link/copy, no append/delta/full transfer) and the sender must send no data, so answer STATUS_DRY_RUN_TRANSFER and stop. - The one exception is a --compare-dest exact hit with no destination copy: a - real run would suppress the data without changing the destination, so it - reports as a skip (STATUS_OK) exactly as the full basis path below would. - Everything read here (destination file, basis candidates) is read-only. */ + + The basis lookup is deliberately content-blind: a real run would only accept + a --compare-dest exact hit after hashing the basis file and comparing it with + the client-supplied digest, which in a dry-run is a 1-bit content oracle. + Under dry_run no basis bytes may be read, so an otherwise-matching entry is + treated as would-transfer instead of a skip. Everything read here (the + destination file's metadata, basis candidates' metadata) is read-only. */ static IncrementalCheckOutcome incremental_check_dry_run_shortcut(IncrementalCheckState* state, bool* skipped, bool* would_transfer) { @@ -1925,9 +1943,12 @@ static IncrementalCheckOutcome incremental_check_dry_run_shortcut(IncrementalChe bool skip_via_compare = false; if (config_has_basis(config) && !config->ignore_times) { BasisMatch basis; + /* hash_content=false: a dry-run must not read or hash the basis file. No + content comparison is possible, so no compare-dest hit can be confirmed + and an otherwise-matching file is reported as would-transfer. */ basis_match_find(config, state->check_path, state->check_size, (time_t)state->check_mtime, (long)state->check_mtime_nsec, state->check_digest, state->check_digest_len, - false, &basis); + false, false, &basis); if (basis.hit && basis.type == BASIS_DEST_COMPARE && !state->has_old_file) skip_via_compare = true; basis_match_free(&basis); @@ -1955,7 +1976,7 @@ static IncrementalCheckOutcome incremental_check_try_basis(IncrementalCheckState BasisMatch basis; basis_match_find(config, state->check_path, state->check_size, (time_t)state->check_mtime, (long)state->check_mtime_nsec, state->check_digest, state->check_digest_len, - true, &basis); + true, true, &basis); if (basis.hit) { if (basis.type == BASIS_DEST_COMPARE) { basis_match_free(&basis); diff --git a/src/shared/xattr.c b/src/shared/xattr.c index 269a867..4cfb363 100644 --- a/src/shared/xattr.c +++ b/src/shared/xattr.c @@ -116,7 +116,7 @@ static bool xattr_name_is_posix_acl(const char* name) { /* ---- SENDER: capture ---- */ -FileXattrList* xattr_capture_path(const char* path) { +FileXattrList* xattr_capture_path(const char* path, bool preserve_acls) { if (!path) return NULL; ssize_t list_size = listxattr(path, NULL, 0); @@ -144,8 +144,9 @@ FileXattrList* xattr_capture_path(const char* path) { break; /* trailing double NUL not expected; stop */ offset += (ssize_t)name_len + 1; /* Capture is sender-side: the scanner has already gated on -X/-A, so the - per-name whitelist here allows the ACL names (true). */ - if (!xattr_name_appliable(name, true)) + per-name whitelist here allows the ACL names only when --acls was + negotiated. Without it a plain -X capture never carries an ACL. */ + if (!xattr_name_appliable(name, preserve_acls)) continue; ssize_t value_size = getxattr(path, name, NULL, 0); if (value_size < 0) diff --git a/src/shared/xattr.h b/src/shared/xattr.h index 5c16c53..8277db1 100644 --- a/src/shared/xattr.h +++ b/src/shared/xattr.h @@ -65,10 +65,13 @@ bool xattr_list_append(FileXattrList* list, const char* name, const void* value, * Used for both capture and receiver-side validation. */ bool xattr_name_appliable(const char* name, bool preserve_acls); -/* Sender: read the whitelisted xattrs of `path` into a new list. Returns NULL - * when the path has no appliable xattrs (or the filesystem has no xattr - * support); an empty-but-valid list is never returned distinct from NULL. */ -FileXattrList* xattr_capture_path(const char* path); +/* Sender: read the whitelisted xattrs of `path` into a new list. The POSIX ACL + * names are captured only when `preserve_acls` (--acls/-A) is set, so a plain + * -X run never carries an ACL it was not asked to preserve; `user.*` is + * unaffected. Returns NULL when the path has no appliable xattrs (or the + * filesystem has no xattr support); an empty-but-valid list is never returned + * distinct from NULL. */ +FileXattrList* xattr_capture_path(const char* path, bool preserve_acls); /* Wire: bounded serialization. xattr_send returns false on write failure; an * empty/NULL list transmits a zero-count block. xattr_receive returns NULL and diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 1c34c00..34f71b0 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -3813,6 +3813,27 @@ class TestBasisDestDirs: assert _read_file(os.path.join(received, self.ADDED)) == \ self._source_tree("c")[self.ADDED], "added file not transferred" + @pytest.mark.ci + def test_dry_run_compare_dest_does_not_read_basis(self, shared_server): + # A dry-run --compare-dest must never read/hash the basis file: doing so + # is a 1-bit content oracle against the client-supplied digest. Even a + # byte-identical basis with a matching size+mtime is therefore reported + # as would-transfer, and nothing is created. + source = self._make_source("basis_dry_src", {self.UNCHANGED: b"stable content v1\n"}) + dest = os.path.join(TEST_DATA_DIR, "basis_dry_dst") + clean_dir(dest) + self._seed_basis(dest, source, "drybasis", {self.UNCHANGED: b"stable content v1\n"}) + before = _snapshot_tree(dest) + result, _ = run_client(source, dest, + flags=["--compare-dest=drybasis", "--dry-run"], + port=shared_server.port) + assert result.returncode == 0, \ + f"dry-run compare-dest failed: {result.stderr[:300]}" + assert self.UNCHANGED in result.stdout, ( + "dry-run compare-dest silently skipped: receiver read the basis content" + ) + assert _snapshot_tree(dest) == before, "dry-run compare-dest mutated the destination" + def test_compare_dest_content_mismatch_forces_transfer(self, shared_server): # The basis holds a file with a DIFFERENT body: even though it shares # the mtime pin, the xxHash check fails and the data must be sent. diff --git a/tests/test_server.c b/tests/test_server.c index 2dff95e..a43e605 100644 --- a/tests/test_server.c +++ b/tests/test_server.c @@ -1,4 +1,5 @@ #include "test_server.h" +#include "checksum.h" #include "config.h" #include "delta.h" #include "file.h" @@ -910,6 +911,92 @@ static void test_incremental_check_fifo_destination_does_not_hang() { } } +/* A server-contacting --dry-run with an alternate basis dir must never read or + hash the basis file. An exact (size+mtime+content) basis match would + otherwise let a client probe the basis bytes against its own supplied digest + (a 1-bit content oracle). The dry-run decision is metadata-only, so even a + byte-identical basis is reported as would-transfer, not a compare-dest skip. */ +static void test_incremental_check_dry_run_basis_does_not_read_content() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->dry_run = true; + char* root = make_check_root("dryb"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + + char basis_dir[1024]; + char basis_path[2048]; + snprintf(basis_dir, sizeof(basis_dir), "%s/basis", root); + EXPECT_EQ_INT(mkdir(basis_dir, 0700), 0); + const char* content = "basis content that matches\n"; + write_check_file(basis_dir, "file.txt", content); + snprintf(basis_path, sizeof(basis_path), "%s/file.txt", basis_dir); + struct stat bst; + EXPECT_EQ_INT(stat(basis_path, &bst), 0); + EXPECT_EQ_INT(config_basis_append(cfg, BASIS_DEST_COMPARE, "basis"), 0); + + /* The (correct) source digest for the basis bytes: an unfixed dry-run would + read+hash the basis and treat this as an exact compare-dest hit. */ + uint8_t digest[CHECKSUM_MAX_DIGEST_LEN]; + size_t digest_len = 0; + EXPECT_TRUE(checksum_digest((ChecksumAlgo)cfg->checksum_algo, cfg->checksum_seed, content, + strlen(content), digest, sizeof(digest), &digest_len)); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(30); + close(p[1]); + io_set_fds(p[0], p[0]); + bool skipped = false; + bool would_transfer = false; + File* file = receive_incremental_check_ex(p[0], cfg, &skipped, &would_transfer); + bool ok = file == NULL && !skipped && would_transfer; + file_destroy(file); + config_delete(cfg); + close(p[0]); + _exit(ok ? 0 : 1); + } else { + close(p[0]); + io_set_fds(p[1], p[1]); + EXPECT_TRUE(send_str(p[1], "file.txt")); + unsigned long long size = (unsigned long long)bst.st_size; + long long mtime = (long long)bst.st_mtime; + long long mtime_nsec = 0; +#ifdef __linux__ + mtime_nsec = (long long)bst.st_mtim.tv_nsec; +#endif + EXPECT_TRUE(send_n_data(p[1], &size, sizeof(size))); + EXPECT_TRUE(send_n_data(p[1], &mtime, sizeof(mtime))); + EXPECT_TRUE(send_n_data(p[1], &mtime_nsec, sizeof(mtime_nsec))); + uint8_t wire_len = (uint8_t)digest_len; + EXPECT_TRUE(send_n_data(p[1], &wire_len, sizeof(wire_len))); + EXPECT_TRUE(send_n_data(p[1], digest, digest_len)); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + /* A skip here would mean the receiver read+hashed the basis file. */ + EXPECT_EQ_INT(s, STATUS_DRY_RUN_TRANSFER); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + /* The dry-run must not have materialized anything in the receive root. */ + char dest_path[2048]; + snprintf(dest_path, sizeof(dest_path), "%s/file.txt", root); + EXPECT_FALSE(file_path_exists_secure(dest_path)); + unlink(basis_path); + rmdir(basis_dir); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + /* B1: a FIFO planted in a --link-dest basis directory must not block * basis_open_regular() either; the basis match is simply declined. */ static void test_incremental_check_basis_fifo_does_not_hang() { @@ -1024,6 +1111,7 @@ void test_server() { test_incremental_check_delta_oversize_reports_failure(); test_incremental_check_fifo_destination_does_not_hang(); test_incremental_check_basis_fifo_does_not_hang(); + test_incremental_check_dry_run_basis_does_not_read_content(); test_receive_manifest_total_entry_cap(); test_late_manifest_abort_frees_keepset(); test_late_manifest_eof_frees_keepset(); diff --git a/tests/test_xattr.c b/tests/test_xattr.c index f82cb3f..c64ff1b 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -273,6 +273,80 @@ static void test_link_copy_fallback_preserves_xattrs() { rmdir(basis_dir); } +/* Capture must honor --acls: xattr_capture_path(path, false) (plain -X) must + * never return the POSIX ACL names, while xattr_capture_path(path, true) (-A) + * does; user.* is captured either way. This is the capture-side counterpart of + * the receiver's --acls gate and must not depend on the caller having checked + * the flag. Guarded on filesystem/ACL support. */ +static void test_xattr_capture_filters_acls() { + const char* path = "test_xattr_capture_acls.txt"; + unlink(path); + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0600); + if (fd < 0) + return; + bool has_xattr = setxattr(path, "user.fastsync.xprobe", "p", 1, 0) == 0; + if (has_xattr) + removexattr(path, "user.fastsync.xprobe"); + if (!has_xattr) { + close(fd); + unlink(path); + return; /* filesystem without xattr support */ + } + if (setxattr(path, "user.keep", "yes", 3, 0) != 0) { + close(fd); + unlink(path); + return; + } + + /* Synthesize a valid non-trivial POSIX access ACL blob (little-endian): + version 2 followed by USER_OBJ/USER/GROUP_OBJ/MASK/OTHER entries. */ + uint32_t acl_uid = geteuid() == 0 ? 65534u : (uint32_t)geteuid(); + unsigned char blob[4 + 5 * 8]; + uint32_t version = 2; + memcpy(blob, &version, 4); + const uint16_t tags[5] = {0x01, 0x02, 0x04, 0x10, 0x20}; /* OBJ/USER/GROUP/MASK/OTHER */ + const uint16_t perms[5] = {0x04, 0x04, 0x04, 0x04, 0x00}; + const uint32_t ids[5] = {0xFFFFFFFFu, acl_uid, 0xFFFFFFFFu, 0xFFFFFFFFu, 0xFFFFFFFFu}; + size_t off = 4; + for (int i = 0; i < 5; i++) { + memcpy(blob + off, &tags[i], sizeof(tags[i])); + off += sizeof(tags[i]); + memcpy(blob + off, &perms[i], sizeof(perms[i])); + off += sizeof(perms[i]); + memcpy(blob + off, &ids[i], sizeof(ids[i])); + off += sizeof(ids[i]); + } + if (setxattr(path, "system.posix_acl_access", blob, off, 0) != 0) { + close(fd); + unlink(path); + return; /* no unprivileged ACL support: skip silently */ + } + close(fd); + + FileXattrList* plain = xattr_capture_path(path, false); + FileXattrList* with_acls = xattr_capture_path(path, true); + bool plain_user = false, plain_acl = false, acl_user = false, acl_acl = false; + for (int i = 0; plain && i < plain->count; i++) { + if (strcmp(plain->items[i].name, "user.keep") == 0) + plain_user = true; + if (strcmp(plain->items[i].name, "system.posix_acl_access") == 0) + plain_acl = true; + } + for (int i = 0; with_acls && i < with_acls->count; i++) { + if (strcmp(with_acls->items[i].name, "user.keep") == 0) + acl_user = true; + if (strcmp(with_acls->items[i].name, "system.posix_acl_access") == 0) + acl_acl = true; + } + EXPECT_TRUE(plain_user); + EXPECT_FALSE(plain_acl); + EXPECT_TRUE(acl_user); + EXPECT_TRUE(acl_acl); + xattr_list_free(plain); + xattr_list_free(with_acls); + unlink(path); +} + /* --fake-super replay: fake_super_store_fd records the source stat into the * reserved xattr, and fake_super_restore_fd re-applies mode/mtime (and owner, * when the process may) fd-relative. Restore must also be a safe no-op with no @@ -409,6 +483,7 @@ void test_xattr() { test_xattr_reject_oversized_value(); test_xattr_count_bound(); test_xattr_capture_and_appliable(); + test_xattr_capture_filters_acls(); test_xattr_receive_drops_acl_without_preserve_acls(); test_link_copy_fallback_preserves_xattrs(); test_fake_super_restore();