From a2370433b2b13ae0bd557de369ab95d0a5ea89cd Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:19:26 +0200 Subject: [PATCH 1/2] fix(receiver): non-blocking receiver opens, inplace type gate, dry-run/B4/B5/B6 Address confirmed receiver security findings B1-B6: B1 (HIGH): add O_NONBLOCK to the three receiver read-opens that opened an existing destination/basis entry before the S_ISREG gate (incremental_check_open_destination, basis_open_regular, hardlink_read_source) so a client-planted FIFO can no longer block the receive thread forever while the post-open type gate still rejects it. B2 (HIGH/MED): --inplace now fstatat(AT_SYMLINK_NOFOLLOW)-probes the target and refuses any existing non-regular entry, opens with O_NONBLOCK, and re-checks S_ISREG on the opened fd. This stops a FIFO from hanging the open and stops a char/block device from being written directly (bypassing --write-devices). B3 (MED): under --dry-run the incremental quick-skip no longer reads/hashes the destination file for --checksum/--delta; it decides from metadata only and reports would-transfer when the comparison is inconclusive, closing the read-only-module content-hash oracle. B4 (LOW): xattr_name_appliable() now gates the two system.posix_acl_* names on preserve_acls (--acls), not the derived use_xattrs (--xattrs OR --acls). The receiver drops (never applies) ACL entries when -A was not negotiated while keeping user.* working for -X. B5 (INFO): receive_manifest_section() charges a per-entry overhead against MAX_MANIFEST_BYTES and the aggregate entry count across all three sections is capped at MAX_MANIFEST_ENTRIES. B6 (MED): data_charge_session() reserves decompressed/chunk-copy bytes against the owning ProtocolSession (MAX_CONNECTION_MEMORY) and records them on the Data so data_destroy() releases them via the Data.owner path. Applied to the whole-file/append/delta decompression sites and chunk_deserialize() per-file copies; a missing session owner degrades to the previous uncharged behavior. Tests: FIFO destination/basis non-hang (with alarm), --inplace FIFO/device refusal, dry-run no-read oracle test plus updated metadata-only dry-run tests, ACL-without--acls drop, manifest total-entry cap, and chunk session charging. --- src/shared/chunk.c | 46 ++++++++ src/shared/chunk.h | 11 ++ src/shared/file.c | 30 ++++- src/shared/file_receive.c | 92 +++++++++++++--- src/shared/xattr.c | 61 ++++++++--- src/shared/xattr.h | 15 ++- tests/fuzz/fuzz_xattr_block.c | 22 ++-- tests/integration/test_features.py | 43 +++++++- tests/test_chunk.c | 48 ++++++++ tests/test_file.c | 91 ++++++++++++++++ tests/test_server.c | 169 +++++++++++++++++++++++++++++ tests/test_xattr.c | 70 ++++++++++-- 12 files changed, 635 insertions(+), 63 deletions(-) diff --git a/src/shared/chunk.c b/src/shared/chunk.c index 1f3be3a..ab0fb4c 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,33 @@ #define MAX_FILE_DATA_SIZE (64ULL * 1024 * 1024) #define MAX_FILES_PER_CHUNK 65536U +/* Reserve `charge` against `session`'s connection budget. This mirrors the + static protocol_reserve_memory() in protocol.c: the receive-side call sites + only have the Data.owner pointer (a ProtocolSession*), and protocol.c is out + of scope for this fix, so the same atomic CAS accounting is reproduced here. + The matching release always goes through data_destroy()'s Data.owner path. */ +static bool chunk_session_reserve(ProtocolSession* session, size_t charge) { + unsigned long long allocated = atomic_load(&session->total_allocated_bytes); + while (true) { + if (allocated > MAX_CONNECTION_MEMORY || + (unsigned long long)charge > MAX_CONNECTION_MEMORY - allocated) + return false; + if (atomic_compare_exchange_weak(&session->total_allocated_bytes, &allocated, + allocated + (unsigned long long)charge)) + return true; + } +} + +bool data_charge_session(Data* data, ProtocolSession* session, size_t charge) { + if (!data || charge == 0 || session == NULL) + return true; + if (!chunk_session_reserve(session, charge)) + return false; + data->owner = session; + data->protocol_charge = charge; + return true; +} + Chunk* chunk_create(File** items, int element_count) { if (element_count < 0 || (element_count > 0 && items == NULL)) return NULL; @@ -370,6 +398,15 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { Data* replacement = data_create(file_data, file_data_size); if (replacement == NULL) goto error; + /* Charge the retained per-file copy to the connection budget (when the + inbound chunk carries an owning session) so the queued copies are not + held outside MAX_CONNECTION_MEMORY (B6). A NULL owner (e.g. a local + batch apply) leaves the copy uncharged. */ + if (!data_charge_session(replacement, data->owner, allocation_size)) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded for chunk file data"); + data_destroy(replacement); + goto error; + } data_destroy(file->data); file->data = replacement; data_pointer += file_data_size; @@ -466,12 +503,21 @@ Chunk* receive_chunk_data(int fd, const Config* config) { } Data* data_to_process = chunk_data; if (config->use_compression) { + /* Preserve the inbound session across decompression so the (larger) + decompressed chunk is charged to the same connection budget; the + compressed buffer's own charge is released by data_destroy below. */ + ProtocolSession* owner = chunk_data->owner; data_to_process = data_decompress_limited(chunk_data, MAX_CHUNK_SIZE); data_destroy(chunk_data); if (data_to_process == NULL) { log_message(LOG_LEVEL_ERROR, "Failed to decompress chunk"); return NULL; } + if (!data_charge_session(data_to_process, owner, data_to_process->size)) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded for decompressed chunk"); + data_destroy(data_to_process); + return NULL; + } } // Reject chunks larger than the maximum allowed size to prevent OOM. diff --git a/src/shared/chunk.h b/src/shared/chunk.h index 2c04e85..65202ad 100644 --- a/src/shared/chunk.h +++ b/src/shared/chunk.h @@ -23,4 +23,15 @@ Data* chunk_compress_with_threads(Chunk* chunk, int compression_level, bool use_ int compression_threads); Chunk* receive_chunk_data(int fd, const Config* config); +/* Charge `charge` retained bytes of `data` against `session`'s per-connection + * budget (MAX_CONNECTION_MEMORY), mirroring the protocol layer's accounting, and + * record them on `data` so data_destroy() returns the charge through the + * Data.owner path. Returns false (leaving `data` uncharged) when the ceiling + * would be exceeded. A NULL/zero-size charge or a NULL session is a no-op + * success. The receive-side decompression and chunk-copy paths know the owning + * session only through the Data.owner of the buffer they are processing, so + * this is the entry point that lets them participate in the connection budget + * without a session handle (B6). */ +bool data_charge_session(Data* data, ProtocolSession* session, size_t charge); + #endif diff --git a/src/shared/file.c b/src/shared/file.c index 9d5aa23..47b13ab 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -890,13 +890,35 @@ static bool file_to_disk_secure_impl(const char* path, const void* data, if (inplace) { /* --inplace writes directly into the destination; a scratch --temp-dir does not apply and must never redirect these writes. */ - fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW, 0644); + /* Type gate BEFORE opening: an existing destination entry that is not a + regular file (FIFO, socket, char/block device, directory) must never be + opened for writing. Opening a FIFO would block the receive thread + forever and writing into a device would bypass the --write-devices / + super-mode gate (a client-controlled device write). fstatat with + AT_SYMLINK_NOFOLLOW does not follow a symlink and does not block. */ + struct stat pre_stat; + if (fstatat(dirfd, leaf, &pre_stat, AT_SYMLINK_NOFOLLOW) == 0 && !S_ISREG(pre_stat.st_mode)) { + close(dirfd); + free(leaf); + return false; + } + /* O_NONBLOCK: a no-op for a regular file, but a raced-in FIFO cannot block + the open before the post-open S_ISREG re-check rejects it. */ + fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK, 0644); if (fd >= 0) { struct stat destination_stat; + /* Re-check the opened descriptor: a concurrent replacement between the + fstatat probe and the open (or a device/FIFO raced in) must never be + written through. */ + if (fstat(fd, &destination_stat) != 0 || !S_ISREG(destination_stat.st_mode)) { + close(fd); + close(dirfd); + free(leaf); + return false; + } bool newer = false; - if (update && metadata && fstat(fd, &destination_stat) == 0 && - S_ISREG(destination_stat.st_mode)) { - newer = stat_is_newer(&destination_stat, metadata); + if (update && metadata && stat_is_newer(&destination_stat, metadata)) { + newer = true; } if (newer) { ok = true; diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index fc2bd44..8e3c2c7 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -12,6 +12,7 @@ #include "array_list.h" #include "charset.h" #include "chmod.h" +#include "chunk.h" #include "compression.h" #include "config.h" #include "data.h" @@ -27,6 +28,11 @@ #define MAX_SERVER_DELETE_COUNT 100000U #define MAX_FILE_DATA_SIZE MAX_RECEIVE_WHOLE_FILE_SIZE +/* Retained cost of one delete-manifest entry beyond its path bytes: the + ArrayList pointer slot plus an approximate malloc header/rounding for the + heap copy. Charged against MAX_MANIFEST_BYTES so a frame full of tiny paths + cannot retain far more than the byte budget (B5). */ +#define MANIFEST_ENTRY_OVERHEAD (sizeof(char*) + 16) bool file_save_to_disk(const char* root_directory, const File* file, const Config* config) { return file_save_to_disk_full(root_directory, file, config) != FILE_SAVE_ERROR; @@ -127,7 +133,10 @@ static bool hardlink_read_source(const char* path, void** out_buf, unsigned long *source_absent = errno == ENOENT || errno == ENOTDIR; return false; } - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK is a no-op for a regular file but makes openat() fail/succeed + immediately for a client-planted FIFO instead of blocking the receive + thread forever; the post-open S_ISREG gate below is the actual type check. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); int saved_errno = errno; free(leaf); close(parent_fd); @@ -944,7 +953,7 @@ static bool receive_file_xattrs(File* file, int fd, const Config* config) { if (!config->use_xattrs) return true; int xok = 0; - FileXattrList* list = xattr_receive(fd, &xok); + FileXattrList* list = xattr_receive(fd, &xok, config->preserve_acls); if (!xok) { xattr_list_free(list); return false; @@ -1009,6 +1018,7 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ !compression_should_skip_with_suffixes( check_path, config->skip_compress_suffixes, config->skip_compress_set ? config->skip_compress_count : -1)) { + ProtocolSession* owner = delta_data->owner; raw_delta = data_decompress_limited(delta_data, MAX_RECEIVE_WHOLE_FILE_SIZE); data_destroy(delta_data); if (!raw_delta) { @@ -1017,6 +1027,15 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ *failed = true; return NULL; } + /* Charge the decompressed delta to the connection budget (the paired + wire buffer's charge was just released). */ + if (!data_charge_session(raw_delta, owner, raw_delta->size)) { + data_destroy(raw_delta); + free(old_data); + delta_signature_destroy(sig); + *failed = true; + return NULL; + } } Delta* delta = delta_deserialize(raw_delta); @@ -1131,12 +1150,20 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ file->path, config->skip_compress_suffixes, config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (uncompressed == NULL) { file_destroy(file); *failed = true; return NULL; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + file_destroy(file); + send_status(fd, STATUS_ERROR); + *failed = true; + return NULL; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); file_destroy(file); @@ -1193,7 +1220,9 @@ static bool basis_open_regular(const char* path, unsigned long long expected_siz int parent_fd = file_open_secure_parent(path, &leaf, false); if (parent_fd < 0) return false; - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK: a client-planted FIFO must not block the receiver's openat() + forever; the fstat()/S_ISREG gate below rejects it immediately. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); free(leaf); close(parent_fd); if (fd < 0) @@ -1637,11 +1666,17 @@ static File* receive_full_file(int fd, const Config* config, const char* path) { config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (uncompressed == NULL) { file_destroy(file); return NULL; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + file_destroy(file); + return NULL; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); file_destroy(file); @@ -1776,7 +1811,9 @@ static IncrementalCheckOutcome incremental_check_open_destination(IncrementalChe char* leaf = NULL; int parent_fd = file_open_secure_parent(full_path, &leaf, false); if (parent_fd >= 0) { - state->old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK: an existing FIFO at the destination must not block this + openat(); the S_ISREG gate below rejects the non-regular entry. */ + state->old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); free(leaf); close(parent_fd); state->has_old_file = state->old_fd >= 0 && fstat(state->old_fd, &state->old_st) == 0 && @@ -1815,7 +1852,15 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat bool try_delta = config->use_delta && !config->whole_file && has_old_file && delta_should_attempt(old_size, state->check_size, config->delta_max_file_size); bool checksum_needs_read = size_equal && !config->ignore_times && config->checksum; - bool need_old_data = checksum_needs_read || try_delta; + /* --dry-run must never read the destination file's CONTENTS: a client could + otherwise use `--dry-run --checksum` against a read-only module as a + 1-bit content oracle (hash match / mismatch) and force arbitrary reads. + Decide from metadata alone; when metadata is inconclusive (checksum or + delta would have required the body) report would-transfer. The real + (non-dry-run) behavior below is unchanged. */ + bool need_old_data = !config->dry_run && (checksum_needs_read || try_delta); + if (config->dry_run) + try_delta = false; *out_try_delta = try_delta; if (need_old_data && has_old_file && old_size > 0 && old_size <= MAX_RECEIVE_WHOLE_FILE_SIZE && @@ -1836,7 +1881,12 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat } bool match = false; - if (checksum_needs_read) { + if (config->dry_run) { + /* Metadata-only decision: a size match plus a matching mtime is treated as + up to date; --checksum/--delta cannot be verified without reading, so an + otherwise inconclusive comparison is a would-transfer. */ + match = size_equal && !config->ignore_times && (config->size_only || match_by_metadata); + } else if (checksum_needs_read) { uint8_t old_digest[CHECKSUM_MAX_DIGEST_LEN]; size_t old_len = 0; bool hashed = checksum_digest((ChecksumAlgo)config->checksum_algo, config->checksum_seed, @@ -2050,7 +2100,7 @@ static IncrementalCheckOutcome incremental_check_try_append_resume(IncrementalCh } if (config->use_xattrs) { int xok = 0; - append_xattrs = xattr_receive(fd, &xok); + append_xattrs = xattr_receive(fd, &xok, config->preserve_acls); if (!xok) { xattr_list_free(append_xattrs); return INCREMENTAL_ERROR; @@ -2066,11 +2116,17 @@ static IncrementalCheckOutcome incremental_check_try_append_resume(IncrementalCh config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(tail, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = tail->owner; data_destroy(tail); if (uncompressed == NULL) { xattr_list_free(append_xattrs); return INCREMENTAL_ERROR; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + xattr_list_free(append_xattrs); + return INCREMENTAL_ERROR; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); xattr_list_free(append_xattrs); @@ -2301,11 +2357,17 @@ File* file_receive(const Config* config, int file_descriptor) { config->skip_compress_set ? config->skip_compress_count : -1)) { Data* file_data_uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (file_data_uncompressed == NULL) { file_destroy(file); return NULL; } + if (!data_charge_session(file_data_uncompressed, owner, file_data_uncompressed->size)) { + data_destroy(file_data_uncompressed); + file_destroy(file); + return NULL; + } if (file_data_uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(file_data_uncompressed); file_destroy(file); @@ -2694,19 +2756,21 @@ File* file_receive_special(int file_descriptor) { manifest). Returns an owned DeleteManifest, or NULL after sending STATUS_ERROR when the frame is malformed (bad count, empty/absolute path, path traversal, or an aggregate size beyond MAX_MANIFEST_BYTES). */ -static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_bytes) { +static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_bytes, + size_t* manifest_entries) { int count; if (!receive_int(fd, &count)) { send_status(fd, STATUS_ERROR); return false; } - if (count < 0 || count > MAX_MANIFEST_ENTRIES) { + if (count < 0 || count > MAX_MANIFEST_ENTRIES || + (size_t)count > MAX_MANIFEST_ENTRIES - *manifest_entries) { send_status(fd, STATUS_ERROR); return false; } for (int i = 0; i < count; i++) { char* s = receive_wire_str(fd); - size_t entry_size = s ? strlen(s) : 0; + size_t entry_size = s ? strlen(s) + MANIFEST_ENTRY_OVERHEAD : 0; if (!s || s[0] == '\0' || s[0] == '/' || has_path_traversal(s) || entry_size > MAX_MANIFEST_BYTES - *manifest_bytes || (*manifest_bytes += entry_size) > MAX_MANIFEST_BYTES || !array_list_add(list, s)) { @@ -2715,6 +2779,7 @@ static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_b return false; } } + *manifest_entries += (size_t)count; return true; } @@ -2733,9 +2798,10 @@ DeleteManifest* receive_manifest_entries(int fd) { return NULL; } size_t manifest_bytes = 0; - if (!receive_manifest_section(fd, manifest->keeps, &manifest_bytes) || - !receive_manifest_section(fd, manifest->protected, &manifest_bytes) || - !receive_manifest_section(fd, manifest->missing, &manifest_bytes)) { + size_t manifest_entries = 0; + if (!receive_manifest_section(fd, manifest->keeps, &manifest_bytes, &manifest_entries) || + !receive_manifest_section(fd, manifest->protected, &manifest_bytes, &manifest_entries) || + !receive_manifest_section(fd, manifest->missing, &manifest_bytes, &manifest_entries)) { delete_manifest_free(manifest); return NULL; } diff --git a/src/shared/xattr.c b/src/shared/xattr.c index d01a38b..269a867 100644 --- a/src/shared/xattr.c +++ b/src/shared/xattr.c @@ -73,13 +73,17 @@ bool xattr_list_append(FileXattrList* list, const char* name, const void* value, /* A Linux xattr name is "namespace.name" with an optional leading "trusted.", * "system.", "security.", "user.", or "trusted." prefix. We only ever touch - * the unprivileged "user.*" namespace and the two POSIX ACL xattrs carried in - * the "system." namespace. Everything else -- especially "security.*" (ACLs, - * capabilities, SELinux labels) and "trusted.*" -- is refused so a client can - * never compel the receiver to apply a privileged attribute it would not - * otherwise be able to set (and which would be a local privilege escalation if - * it could). */ -bool xattr_name_appliable(const char* name) { + * the unprivileged "user.*" namespace and, only when --acls/-A was negotiated, + * the two POSIX ACL xattrs carried in the "system." namespace. Everything else + * -- especially "security.*" (ACLs, capabilities, SELinux labels) and + * "trusted.*" -- is refused so a client can never compel the receiver to apply a + * privileged attribute it would not otherwise be able to set (and which would be + * a local privilege escalation if it could). + * + * The ACL gate is deliberate: --xattrs/-X alone derives use_xattrs but must NOT + * authorize the ACL names, otherwise a -X client could plant an ACL the + * receiver never opted into (B4). */ +bool xattr_name_appliable(const char* name, bool preserve_acls) { if (!name || name[0] == '\0') return false; size_t len = strlen(name); @@ -95,12 +99,21 @@ bool xattr_name_appliable(const char* name) { if (strncmp(name, "user.", 5) == 0) return name[5] != '\0'; if (strcmp(name, "system.posix_acl_access") == 0) - return true; + return preserve_acls; if (strcmp(name, "system.posix_acl_default") == 0) - return true; + return preserve_acls; return false; } +/* The two POSIX ACL xattr names: the only names whose applicablity is + * conditional (they require --acls). Used by the receiver to distinguish "not + * negotiated" (drop the entry, keep user.* working for -X) from a genuinely + * disallowed namespace (hard reject). */ +static bool xattr_name_is_posix_acl(const char* name) { + return name != NULL && (strcmp(name, "system.posix_acl_access") == 0 || + strcmp(name, "system.posix_acl_default") == 0); +} + /* ---- SENDER: capture ---- */ FileXattrList* xattr_capture_path(const char* path) { @@ -130,7 +143,9 @@ FileXattrList* xattr_capture_path(const char* path) { if (name_len == 0) break; /* trailing double NUL not expected; stop */ offset += (ssize_t)name_len + 1; - if (!xattr_name_appliable(name)) + /* 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)) continue; ssize_t value_size = getxattr(path, name, NULL, 0); if (value_size < 0) @@ -190,7 +205,7 @@ bool xattr_send(int fd, const FileXattrList* list) { return true; } -FileXattrList* xattr_receive(int fd, int* ok) { +FileXattrList* xattr_receive(int fd, int* ok, bool preserve_acls) { if (ok) *ok = 0; int count; @@ -232,11 +247,19 @@ FileXattrList* xattr_receive(int fd, int* ok) { xattr_list_free(list); return NULL; } - if (!xattr_name_appliable(name)) { - log_message(LOG_LEVEL_ERROR, "rejected xattr block: disallowed namespace for '%s'", name); - free(name); - xattr_list_free(list); - return NULL; + bool skip = false; + if (!xattr_name_appliable(name, preserve_acls)) { + if (!preserve_acls && xattr_name_is_posix_acl(name)) { + /* -X without -A: the sender may still carry ACLs, but the receiver must + never apply an ACL it was not asked to preserve. Consume and drop the + entry (keeping -X compatibility) rather than failing the transfer. */ + skip = true; + } else { + log_message(LOG_LEVEL_ERROR, "rejected xattr block: disallowed namespace for '%s'", name); + free(name); + xattr_list_free(list); + return NULL; + } } int32_t value_len32; if (!receive_n_data(fd, &value_len32, sizeof(value_len32))) { @@ -273,6 +296,12 @@ FileXattrList* xattr_receive(int fd, int* ok) { return NULL; } } + if (skip) { + free(value); + free(name); + budget += (size_t)name_len32 + (size_t)value_len32; + continue; + } if (!xattr_list_append(list, name, value, (size_t)value_len32)) { free(value); free(name); diff --git a/src/shared/xattr.h b/src/shared/xattr.h index 55f22dd..5c16c53 100644 --- a/src/shared/xattr.h +++ b/src/shared/xattr.h @@ -59,9 +59,11 @@ void xattr_list_free(FileXattrList* list); bool xattr_list_append(FileXattrList* list, const char* name, const void* value, size_t value_len); /* True when `name` is a well-formed xattr name AND belongs to a namespace this - * build is authorized to apply (user.* or the two POSIX ACL xattrs). Used for - * both capture and receiver-side validation. */ -bool xattr_name_appliable(const char* name); + * build is authorized to apply. `user.*` is always accepted for -X; the two + * POSIX ACL xattrs are accepted only when `preserve_acls` (--acls/-A) is set, so + * a plain -X run can never carry or apply an ACL the receiver did not ask for. + * 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 @@ -70,9 +72,12 @@ FileXattrList* xattr_capture_path(const char* path); /* Wire: bounded serialization. xattr_send returns false on write failure; an * empty/NULL list transmits a zero-count block. xattr_receive returns NULL and - * sets *ok = 0 on any malformed / oversized / non-whitelisted entry. */ + * sets *ok = 0 on any malformed / oversized / non-whitelisted entry. When + * `preserve_acls` is false, any POSIX ACL entries are consumed and DROPPED (so + * a -X transfer still succeeds and never applies an ACL it did not negotiate); + * a genuinely disallowed namespace is still rejected. */ bool xattr_send(int fd, const FileXattrList* list); -FileXattrList* xattr_receive(int fd, int* ok); +FileXattrList* xattr_receive(int fd, int* ok, bool preserve_acls); /* Receiver: apply every entry fd-relative (fsetxattr) to the just-written file * descriptor. A per-attribute failure (e.g. ACL set refused for non-root on a diff --git a/tests/fuzz/fuzz_xattr_block.c b/tests/fuzz/fuzz_xattr_block.c index 26e3775..f1ba210 100644 --- a/tests/fuzz/fuzz_xattr_block.c +++ b/tests/fuzz/fuzz_xattr_block.c @@ -75,7 +75,7 @@ static void write_best_effort(int fd, const void* data, size_t size) { } static void receive_stream(const unsigned char* prefix, size_t prefix_len, const uint8_t* data, - size_t size) { + size_t size, bool preserve_acls) { int sv[2]; if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) != 0) return; @@ -91,7 +91,7 @@ static void receive_stream(const unsigned char* prefix, size_t prefix_len, const shutdown(sv[0], SHUT_WR); int ok = 0; - FileXattrList* list = xattr_receive(sv[1], &ok); + FileXattrList* list = xattr_receive(sv[1], &ok, preserve_acls); xattr_list_free(list); close(sv[0]); @@ -102,14 +102,18 @@ int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { if (!g_block_ready) build_canonical_block(); - /* Raw bytes as the whole block. */ - receive_stream(NULL, 0, data, size); + /* Raw bytes as the whole block. Exercise both the -X-only (no ACLs) and the + * -A (ACL names accepted) receiver gates. */ + for (int acls = 0; acls < 2; acls++) { + bool preserve_acls = acls != 0; + receive_stream(NULL, 0, data, size, preserve_acls); - /* Valid framing so the fuzzer mutates the entry list, the first value and - * the second entry respectively instead of stopping at the count. */ - receive_stream(g_block, g_off_after_entry0, data, size); - receive_stream(g_block, g_off_value0, data, size); - receive_stream(g_block, g_off_after_count, data, size); + /* Valid framing so the fuzzer mutates the entry list, the first value and + * the second entry respectively instead of stopping at the count. */ + receive_stream(g_block, g_off_after_entry0, data, size, preserve_acls); + receive_stream(g_block, g_off_value0, data, size, preserve_acls); + receive_stream(g_block, g_off_after_count, data, size, preserve_acls); + } return 0; } diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 96372cb..1c34c00 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -444,9 +444,10 @@ class TestRemoteDryRun: self._seed(source) clean_dir(dest) - # Populate the destination with a real transfer, then make exactly one - # file differ (content+size) and add a brand-new file. - result, _ = run_client(source, dest, port=shared_server.port) + # Populate the destination with a real transfer that preserves mtimes + # (--preserve), then make exactly one file differ (content+size) and add + # a brand-new file. + result, _ = run_client(source, dest, flags=["--preserve"], port=shared_server.port) assert result.returncode == 0, f"seed transfer failed: {result.stderr[:200]}" received = get_dest_received_dir(dest, source) @@ -456,9 +457,11 @@ class TestRemoteDryRun: f.write(b"newly added\n") before = _snapshot_tree(received) - # --checksum makes the up-to-date decision content-based (the seed - # transfer did not preserve mtimes), so keep.txt/deep.txt report skip. - result, _ = run_client(source, dest, flags=["--dry-run", "--checksum"], + # --checksum must NOT read destination contents in a dry-run (B3), so + # the up-to-date decision is metadata-only. The --preserve seed made + # keep.txt and deep.txt size+mtime-identical; the dry-run must also + # transmit metadata (--preserve) for that metadata to be comparable. + result, _ = run_client(source, dest, flags=["--dry-run", "--checksum", "--preserve"], port=shared_server.port) assert result.returncode == 0, f"remote dry-run failed: {result.stderr[:300]}" assert "Dry run:" in result.stdout, result.stdout[:200] @@ -470,6 +473,34 @@ class TestRemoteDryRun: assert "deep.txt" not in result.stdout, result.stdout assert _snapshot_tree(received) == before, "remote dry-run mutated the destination" + @pytest.mark.ci + def test_remote_dry_run_checksum_does_not_read_destination(self, shared_server): + """B3: --dry-run --checksum against a read-only module must not read the + destination file's content (a 1-bit hash oracle). A same-size/same-content + file whose mtime differs is therefore reported as would-transfer because + the metadata-only decision is inconclusive, instead of being hashed and + silently skipped.""" + source = os.path.join(TEST_DATA_DIR, "remote_dry_oracle_src") + dest = os.path.join(TEST_DATA_DIR, "remote_dry_oracle_dst") + self._seed(source) + clean_dir(dest) + result, _ = run_client(source, dest, flags=["--preserve"], port=shared_server.port) + assert result.returncode == 0, result.stderr[:200] + received = get_dest_received_dir(dest, source) + + target = os.path.join(received, "keep.txt") + # Identical size and content, but a deliberately different mtime. + os.utime(target, (1000000000, 1000000000)) + before = _snapshot_tree(received) + + result, _ = run_client(source, dest, flags=["--dry-run", "--checksum", "--preserve"], + port=shared_server.port) + assert result.returncode == 0, result.stderr[:300] + assert "keep.txt" in result.stdout, ( + f"dry-run --checksum must not read the destination to prove equality: {result.stdout}" + ) + assert _snapshot_tree(received) == before, "dry-run mutated the destination" + @pytest.mark.ci def test_remote_dry_run_into_empty_dest_creates_nothing(self, shared_server): source = os.path.join(TEST_DATA_DIR, "remote_dry_empty_src") diff --git a/tests/test_chunk.c b/tests/test_chunk.c index b1d6cf6..6266582 100644 --- a/tests/test_chunk.c +++ b/tests/test_chunk.c @@ -1,8 +1,10 @@ #include "chunk.h" +#include "protocol.h" #include "test_utils.h" #include "utils.h" #include +#include #include #include @@ -279,6 +281,51 @@ static void test_chunk_special_rdev_out_of_range_rejected() { chunk_destroy(chunk); } +/* B6: chunk_deserialize() charges each retained per-file copy to the owning + * session's connection budget (MAX_CONNECTION_MEMORY) so queued chunk payloads + * are not held outside the per-connection ceiling; destroying the chunk returns + * the charge through the Data.owner path. */ +static void test_chunk_deserialize_charges_session_budget() { + const char* path = "temp_chunk_charge.txt"; + const char* content = "charge me to the connection budget"; + unlink(path); + file_write_to_disk(path, content, strlen(content), false, false); + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + ProtocolSession session; + protocol_session_init(&session, p[0], p[1]); + protocol_session_set_max_alloc(&session, 4ULL * 1024 * 1024); + + File* f = file_create(path); + EXPECT_NOT_NULL(f); + f->data->size = (unsigned long long)st.st_size; + EXPECT_TRUE(file_load_data(f)); + File* files[1] = {f}; + Chunk* chunk = chunk_create(files, 1); + EXPECT_NOT_NULL(chunk); + Data* serialized = chunk_serialize(chunk, false); + EXPECT_NOT_NULL(serialized); + /* Simulate a received buffer carrying its owning session. */ + serialized->owner = &session; + + Chunk* deserialized = chunk_deserialize(serialized, false); + EXPECT_NOT_NULL(deserialized); + unsigned long long charged = atomic_load(&session.total_allocated_bytes); + EXPECT_EQ_INT((int)charged, (int)strlen(content)); + chunk_destroy(deserialized); + /* The copy's charge is released with the File/Data on destroy. */ + EXPECT_EQ_INT((int)atomic_load(&session.total_allocated_bytes), 0); + + data_destroy(serialized); + chunk_destroy(chunk); + close(p[0]); + close(p[1]); + unlink(path); +} + void test_chunk() { test_file_operations(); test_chunk_operations(); @@ -286,4 +333,5 @@ void test_chunk() { test_chunk_symlink_roundtrip(); test_chunk_special_rdev_roundtrip(); test_chunk_special_rdev_out_of_range_rejected(); + test_chunk_deserialize_charges_session_budget(); } diff --git a/tests/test_file.c b/tests/test_file.c index 73c3431..37fe83e 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -994,6 +995,94 @@ static void test_inplace_overwrite_truncates_shorter_payload() { rmdir(root); } +/* B2: --inplace must refuse an existing non-regular destination entry. A FIFO + would block open(O_WRONLY) forever and a device node would be written + directly, bypassing the --write-devices/super gate. Forked with an alarm so + a regression is a prompt failure instead of a hung suite. */ +static void test_inplace_refuses_fifo_destination() { + const char* root = "test_inplace_fifo_tmp"; + const char* path = "test_inplace_fifo_tmp/fifo"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + EXPECT_EQ_INT(mkfifo(path, 0600), 0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(10); + File* f = file_create("fifo"); + if (!f) + _exit(1); + const char* content = "payload"; + f->data->data = malloc(strlen(content)); + if (!f->data->data) + _exit(1); + memcpy(f->data->data, content, strlen(content)); + f->data->size = strlen(content); + Config* cfg = config_create(); + if (!cfg) + _exit(1); + cfg->inplace = true; + bool written = file_save_to_disk(root, f, cfg); + file_destroy(f); + config_delete(cfg); + _exit(written ? 1 : 0); /* must be refused */ + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + struct stat st; + EXPECT_EQ_INT(lstat(path, &st), 0); + EXPECT_TRUE(S_ISFIFO(st.st_mode)); /* left untouched */ + unlink(path); + rmdir(root); +} + +/* B2: an existing char device must not be written by --inplace. mknod needs + privilege, so a non-root run skips gracefully. /dev/null's (1:3) rdev makes + the negative case harmless if it ever regresses. */ +static void test_inplace_refuses_device_destination() { + const char* root = "test_inplace_dev_tmp"; + const char* path = "test_inplace_dev_tmp/dev"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + if (mknod(path, S_IFCHR | 0600, makedev(1, 3)) != 0) { + rmdir(root); + return; /* no privilege to create a device node: skip */ + } + + pid_t pid = fork(); + if (pid == 0) { + alarm(10); + File* f = file_create("dev"); + if (!f) + _exit(1); + const char* content = "payload"; + f->data->data = malloc(strlen(content)); + if (!f->data->data) + _exit(1); + memcpy(f->data->data, content, strlen(content)); + f->data->size = strlen(content); + Config* cfg = config_create(); + if (!cfg) + _exit(1); + cfg->inplace = true; + bool written = file_save_to_disk(root, f, cfg); + file_destroy(f); + config_delete(cfg); + _exit(written ? 1 : 0); /* must be refused */ + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + struct stat st; + EXPECT_EQ_INT(lstat(path, &st), 0); + EXPECT_TRUE(S_ISCHR(st.st_mode)); /* still a device, not replaced */ + unlink(path); + rmdir(root); +} + /* Explicit directory entries (--dirs) create the directory under the receive root through the same save funnel, creating parents as needed, and reject traversal the same way a file path does. */ @@ -1606,4 +1695,6 @@ void test_file() { test_inplace_overwrite_clears_special_mode_bits(); test_inplace_overwrite_metadata_strips_special_bits(); test_inplace_overwrite_truncates_shorter_payload(); + test_inplace_refuses_fifo_destination(); + test_inplace_refuses_device_destination(); } diff --git a/tests/test_server.c b/tests/test_server.c index b2246e8..2dff95e 100644 --- a/tests/test_server.c +++ b/tests/test_server.c @@ -844,6 +844,172 @@ static void test_special_socket_path_log_escaped() { EXPECT_NOT_NULL(strstr(output, "socket not recreated: evil\\#012path")); } +/* B1: a client-planted FIFO at the destination must not block the receiver's + * incremental-check open. With the O_NONBLOCK open plus the post-open S_ISREG + * gate the FIFO is simply "no existing regular file", so the receiver proceeds + * to a full transfer; without O_NONBLOCK the child blocks in openat() and the + * alarm(30) kills it. */ +static void test_incremental_check_fifo_destination_does_not_hang() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + char* root = make_check_root("qffo"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + char path[1024]; + snprintf(path, sizeof(path), "%s/file.txt", root); + EXPECT_EQ_INT(mkfifo(path, 0600), 0); + + 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; + File* file = receive_incremental_check(p[0], cfg, &skipped); + bool ok = file != NULL && !skipped; + 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 = 4; + long long mtime = 42; + long long mtime_nsec = 0; + 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))); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + EXPECT_EQ_INT(s, STATUS_NEXT); + + Data* body = data_create_reserve(4); + EXPECT_NOT_NULL(body); + body->data = malloc(4); + EXPECT_NOT_NULL(body->data); + memcpy(body->data, "data", 4); + body->size = 4; + EXPECT_TRUE(send_data(p[1], body)); + data_destroy(body); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + unlink(path); + 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() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + char* root = make_check_root("qbfi"); + 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); + snprintf(basis_path, sizeof(basis_path), "%s/file.txt", basis_dir); + EXPECT_EQ_INT(mkfifo(basis_path, 0600), 0); + EXPECT_EQ_INT(config_basis_append(cfg, BASIS_DEST_LINK, "basis"), 0); + + 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; + File* file = receive_incremental_check(p[0], cfg, &skipped); + bool ok = file != NULL && !skipped; + 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 = 4; + long long mtime = 42; + long long mtime_nsec = 0; + 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))); + /* config_has_basis() makes the request carry the source digest. */ + uint8_t wire_len = 8; + uint8_t digest[8] = {0}; + EXPECT_TRUE(send_n_data(p[1], &wire_len, sizeof(wire_len))); + EXPECT_TRUE(send_n_data(p[1], digest, sizeof(digest))); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + EXPECT_EQ_INT(s, STATUS_NEXT); + + Data* body = data_create_reserve(4); + EXPECT_NOT_NULL(body); + body->data = malloc(4); + EXPECT_NOT_NULL(body->data); + memcpy(body->data, "data", 4); + body->size = 4; + EXPECT_TRUE(send_data(p[1], body)); + data_destroy(body); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + unlink(basis_path); + rmdir(basis_dir); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + +/* B5: the aggregate entry count across the three manifest sections is capped at + * MAX_MANIFEST_ENTRIES, and a section that would push the total over the cap is + * rejected before its entries are read (so a tiny first section followed by a + * huge claimed second section fails fast). */ +static void test_receive_manifest_total_entry_cap() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->receive_root_directory = str_dup("/tmp/dst"); + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + + EXPECT_TRUE(send_int(p[1], 1)); + EXPECT_TRUE(send_str(p[1], "keep.txt")); + /* The second section alone is within its per-section cap, but 1 + it exceeds + the cross-section cap; the receiver must reject at the count. */ + EXPECT_TRUE(send_int(p[1], MAX_MANIFEST_ENTRIES)); + EXPECT_NULL(receive_manifest_entries(p[0])); + Status status; + EXPECT_TRUE(receive_status(p[1], &status)); + EXPECT_EQ_INT(status, STATUS_ERROR); + + close(p[0]); + close(p[1]); + config_delete(cfg); +} + void test_server() { test_special_socket_path_log_escaped(); if (!is_running_under_valgrind()) { @@ -856,6 +1022,9 @@ void test_server() { test_incremental_check_size_mismatch_full_transfer(); test_incremental_check_dry_run_reports_transfer_without_writing(); test_incremental_check_delta_oversize_reports_failure(); + test_incremental_check_fifo_destination_does_not_hang(); + test_incremental_check_basis_fifo_does_not_hang(); + test_receive_manifest_total_entry_cap(); test_late_manifest_abort_frees_keepset(); test_late_manifest_eof_frees_keepset(); test_late_second_manifest_frees_both(); diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 4997ae2..f82cb3f 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -17,7 +17,7 @@ static void run_recv_helper(int fd) { int ok = 0; - FileXattrList* list = xattr_receive(fd, &ok); + FileXattrList* list = xattr_receive(fd, &ok, false); if (!ok) _exit(1); if (!list) { @@ -64,7 +64,7 @@ static void test_xattr_wire_roundtrip() { static void run_recv_must_fail(int fd) { int ok = 0; - FileXattrList* list = xattr_receive(fd, &ok); + FileXattrList* list = xattr_receive(fd, &ok, false); /* A NULL list with ok==0 is the expected rejection. */ if (ok == 0 && list == NULL) _exit(0); @@ -148,15 +148,64 @@ static void test_xattr_count_bound() { /* The captured list on a plain file reflects only whitelisted namespaces * (Linux only; skipped when the filesystem has no xattr support). */ static void test_xattr_capture_and_appliable() { - EXPECT_FALSE(xattr_name_appliable(NULL)); - EXPECT_FALSE(xattr_name_appliable("")); - EXPECT_FALSE(xattr_name_appliable("security.selinux")); - EXPECT_FALSE(xattr_name_appliable("trusted.blob")); - EXPECT_TRUE(xattr_name_appliable("user.foo")); + EXPECT_FALSE(xattr_name_appliable(NULL, false)); + EXPECT_FALSE(xattr_name_appliable("", false)); + EXPECT_FALSE(xattr_name_appliable("security.selinux", false)); + EXPECT_FALSE(xattr_name_appliable("trusted.blob", false)); + EXPECT_TRUE(xattr_name_appliable("user.foo", false)); + EXPECT_TRUE(xattr_name_appliable("user.foo", true)); /* The reserved fake-super key is receiver-only and never forwarded/applied. */ - EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat")); - EXPECT_TRUE(xattr_name_appliable("system.posix_acl_access")); - EXPECT_TRUE(xattr_name_appliable("system.posix_acl_default")); + EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat", false)); + EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat", true)); + /* B4: the ACL names require --acls; -X alone must not authorize them. */ + EXPECT_FALSE(xattr_name_appliable("system.posix_acl_access", false)); + EXPECT_FALSE(xattr_name_appliable("system.posix_acl_default", false)); + EXPECT_TRUE(xattr_name_appliable("system.posix_acl_access", true)); + EXPECT_TRUE(xattr_name_appliable("system.posix_acl_default", true)); +} + +/* B4: a `-X`-only receiver (preserve_acls false) must NOT apply an incoming + * ACL xattr, while a user.* attribute in the same block still survives. The + * ACL entry is dropped, not applied (and the -X transfer is not failed). */ +static void run_recv_drops_acl_keeps_user(int fd) { + int ok = 0; + FileXattrList* list = xattr_receive(fd, &ok, false); + if (!ok || list == NULL) + _exit(1); + bool saw_user = false; + for (int i = 0; i < list->count; i++) { + if (strcmp(list->items[i].name, "system.posix_acl_access") == 0) + _exit(1); /* ACL must have been dropped */ + if (strcmp(list->items[i].name, "user.keep") == 0) + saw_user = true; + } + xattr_list_free(list); + _exit(saw_user ? 0 : 1); +} + +static void test_xattr_receive_drops_acl_without_preserve_acls() { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + pid_t pid = fork(); + if (pid == 0) { + close(p[1]); + io_set_fds(p[0], p[0]); + run_recv_drops_acl_keeps_user(p[0]); + } + close(p[0]); + io_set_fds(p[1], p[1]); + FileXattrList* list = xattr_list_new(); + EXPECT_NOT_NULL(list); + EXPECT_TRUE(xattr_list_append(list, "system.posix_acl_access", "\x02\x00\x00\x00", 4)); + EXPECT_TRUE(xattr_list_append(list, "user.keep", "yes", 3)); + xattr_send(p[1], list); + xattr_list_free(list); + int status; + waitpid(pid, &status, 0); + close(p[1]); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); } /* MINOR-2: a --link-dest / -H copy fallback (linkat refused) must still apply @@ -360,6 +409,7 @@ void test_xattr() { test_xattr_reject_oversized_value(); test_xattr_count_bound(); test_xattr_capture_and_appliable(); + test_xattr_receive_drops_acl_without_preserve_acls(); test_link_copy_fallback_preserves_xattrs(); test_fake_super_restore(); test_fake_super_owner_gate(); From 5893de4a34ada1a2b19e159b1d3389ba2375929b Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 17:19:27 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(receiver):=20close=20re-review=20findin?= =?UTF-8?q?gs=20=E2=80=94=20dry-run=20basis=20oracle,=20ACL=20capture,=20f?= =?UTF-8?q?sync=20reopen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to a237043 addressing three security/correctness re-review findings. (1) MEDIUM: a server-contacting --dry-run with --compare-dest/--copy-dest/ --link-dest still read and hashed the basis file and compared it with the client-supplied digest, a 1-bit content oracle. basis_match_find() gains a hash_content parameter; the dry-run shortcut passes false and returns no match without touching basis bytes, so an otherwise-matching entry is reported as would-transfer. The real (non-dry-run) path is unchanged. (2) LOW: xattr_capture_path() hardcoded preserve_acls=true, so the receiver's hard-link copy fallback re-applied system.posix_acl_* even when -A was not negotiated. The function now takes preserve_acls and members.* is unaffected; scanner and receiver callers thread the negotiated flag. (3) INFO: the --fsync --link-dest temp reopen now uses O_NONBLOCK and treats a raced-in FIFO's ENXIO as a benign fsync-skip instead of blocking. Tests: dry-run + basis unit test (asserts would-transfer, no content read) and integration test; xattr capture ACL-filter test. Verified strict build, ASan, clang-format, cppcheck, and the CI integration subset. --- src/client/scanner.c | 4 +- src/shared/file.c | 16 ++++-- src/shared/file_receive.c | 41 ++++++++++---- src/shared/xattr.c | 7 ++- src/shared/xattr.h | 11 ++-- tests/integration/test_features.py | 21 +++++++ tests/test_server.c | 88 ++++++++++++++++++++++++++++++ tests/test_xattr.c | 75 +++++++++++++++++++++++++ 8 files changed, 240 insertions(+), 23 deletions(-) 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();