fix: review pass — uninit stats, append crash, filter rollback, ASan leak
CI / lint (pull_request) Successful in 1m45s
CI / sanitizers (address) (pull_request) Skipped
CI / sanitizers (undefined) (pull_request) Skipped
CI / fuzz-build (pull_request) Skipped
CI / coverage (pull_request) Skipped
CI / valgrind (pull_request) Skipped
CI / build-and-test (pull_request) Successful in 45s
CI / lint (pull_request) Successful in 1m45s
CI / sanitizers (address) (pull_request) Skipped
CI / sanitizers (undefined) (pull_request) Skipped
CI / fuzz-build (pull_request) Skipped
CI / coverage (pull_request) Skipped
CI / valgrind (pull_request) Skipped
CI / build-and-test (pull_request) Successful in 45s
Address findings from the four-agent review of PR #298: - file_create: zero the new File.matched_bytes. It was uninitialized malloc memory, so the receiver could sum a garbage value into STATUS_STATS "Matched data" (nondeterministic --stats divergence and an uninitialized-heap disclosure on the wire). - send_append: load the source into memory before hashing/copying the prefix and tail. Files >64 MiB without compression (and --sendfile runs) are streamed without loading, so --append/--append-verify dereferenced a NULL data->data and crashed. - filter_file_append: clamp the rollback to the surviving rule count. A "clear" rule in a merge file frees every rule including the caller's; the old rollback rewound count to rules_before and resurrected freed pointers for a double free / UAF. Also roll back when set_rule_owner fails instead of leaving owner-less rules. - filter_rule_parse: reject the xattr-name filter modifier (x), which was parsed and silently reinterpreted as a filename rule (affecting what --delete protects). The p modifier stays accepted (existing grammar test). - Remove two dead functions: compression_default_algo and change_render_itemize_code. - tests: free ctx->would_delete in the two test_multiprocessing manual teardowns (ASan leak, 1648 bytes/run). - docs: correct the RSYNC_COMPAT/HANDOFF tally (156 rows: 106/27/23), downgrade --info/--debug to caveat with their silent categories, add %C-vs-xxh64 and --delete-delay count caveats, refresh stale xattr mode comments, and document -p special-bit (setuid/setgid/sticky) parity plus its mitigations.
This commit is contained in:
@@ -158,14 +158,6 @@ static void itemize_code(const Config* config, const ChangeEvent* event, char co
|
||||
code[11] = '\0';
|
||||
}
|
||||
|
||||
char* change_render_itemize_code(const Config* config, const ChangeEvent* event) {
|
||||
if (event == NULL || event->decision != CHANGE_SENT)
|
||||
return str_dup("");
|
||||
char code[12];
|
||||
itemize_code(config, event, code);
|
||||
return str_dup(code);
|
||||
}
|
||||
|
||||
/* rsync %n: the transfer-relative name, with a trailing slash for directories. */
|
||||
static bool append_name(StrBuf* buf, const ChangeEvent* event) {
|
||||
if (!strbuf_append(buf, event->name != NULL ? event->name : ""))
|
||||
|
||||
@@ -63,9 +63,6 @@ bool change_list_enabled(const Config* config);
|
||||
* (`%i %n%L`): `>f+++++++++ sub/b.txt`. Caller frees the result. */
|
||||
char* change_render_itemize(const Config* config, const ChangeEvent* event);
|
||||
|
||||
/* Render only the 11-character itemize code (rsync %i). Caller frees. */
|
||||
char* change_render_itemize_code(const Config* config, const ChangeEvent* event);
|
||||
|
||||
/* Expand an --out-format/--log-file-format template. Supported tokens:
|
||||
* %i itemize code %n transfer-relative name (dir: trailing /)
|
||||
* %f long display path %l file length in bytes
|
||||
|
||||
@@ -1666,6 +1666,10 @@ static int send_delta(Client* client, File* file, DeltaSignature* sig, Config* c
|
||||
static int send_append(const Client* client, File* file, Config* config,
|
||||
unsigned long long offset) {
|
||||
int fd = client->file_descriptor;
|
||||
if (file->data->data == NULL && !file_load_data(file)) {
|
||||
send_status(fd, STATUS_ERROR);
|
||||
return -1;
|
||||
}
|
||||
const unsigned long long fsize = file->data->size;
|
||||
if (offset >= fsize) {
|
||||
send_status(fd, STATUS_ERROR);
|
||||
|
||||
@@ -77,10 +77,6 @@ bool compression_should_skip_with_suffixes(const char* path, char* const* suffix
|
||||
return false;
|
||||
}
|
||||
|
||||
CompressionAlgo compression_default_algo(void) {
|
||||
return COMPRESSION_ALGO_ZSTD;
|
||||
}
|
||||
|
||||
int compression_algo_from_name(const char* name) {
|
||||
if (!name)
|
||||
return -1;
|
||||
|
||||
@@ -22,8 +22,6 @@ typedef enum {
|
||||
COMPRESSION_ALGO_ZLIBX = 4
|
||||
} CompressionAlgo;
|
||||
|
||||
CompressionAlgo compression_default_algo(void);
|
||||
|
||||
/* Resolve a --compress-choice string (case-insensitive) to an algorithm id.
|
||||
* Accepts "zstd", "lz4", "zlib", "zlibx", "none". "auto" is not an algorithm
|
||||
* here; the caller resolves it to the negotiated default. Returns -1 for any
|
||||
|
||||
@@ -186,6 +186,7 @@ File* file_create(const char* path) {
|
||||
file->rdev_minor = 0;
|
||||
file->xattrs = NULL;
|
||||
file->dest_state = (OutputDestState){0};
|
||||
file->matched_bytes = 0;
|
||||
return file;
|
||||
}
|
||||
|
||||
|
||||
+20
-8
@@ -301,6 +301,10 @@ FilterRule* filter_rule_parse(const char* line, const FilterParseOptions* opts,
|
||||
filter_set_error(err, err_size, "the C modifier is handled by the rule-list parser");
|
||||
return NULL;
|
||||
}
|
||||
if (xattr) {
|
||||
filter_set_error(err, err_size, "xattr-name filter rules (the x modifier) are not supported");
|
||||
return NULL;
|
||||
}
|
||||
if (kind == RULE_KIND_MERGE || kind == RULE_KIND_DIR_MERGE) {
|
||||
filter_set_error(err, err_size, "merge/dir-merge rules are handled by the rule-list parser");
|
||||
return NULL;
|
||||
@@ -637,6 +641,20 @@ FilterRuleList* filter_base_build(const char* const* rule_texts, int rule_count,
|
||||
|
||||
/* ---- Per-directory merge files ---- */
|
||||
|
||||
/* Undo the rules and dir-merge registrations that one merge file appended,
|
||||
* leaving the caller's earlier content intact. A "clear" rule inside the file
|
||||
* frees every rule, including the caller's; clamp to the surviving count so
|
||||
* those already-freed rules are never resurrected and freed a second time. */
|
||||
static void filter_file_rollback(FilterRuleList* list, int rules_before, int dir_merges_before) {
|
||||
int first = rules_before < list->count ? rules_before : list->count;
|
||||
for (int i = first; i < list->count; i++)
|
||||
filter_rule_free(list->items[i]);
|
||||
list->count = first;
|
||||
for (int i = dir_merges_before; i < list->dir_merge_count; i++)
|
||||
free(list->dir_merge_names[i]);
|
||||
list->dir_merge_count = dir_merges_before;
|
||||
}
|
||||
|
||||
bool filter_file_append(FilterRuleList* list, const char* dir_path, const char* name,
|
||||
const char* owner_rel, const FilterParseOptions* opts, bool* exists,
|
||||
char* err, size_t err_size) {
|
||||
@@ -698,19 +716,13 @@ bool filter_file_append(FilterRuleList* list, const char* dir_path, const char*
|
||||
free(line);
|
||||
fclose(fp);
|
||||
if (!ok) {
|
||||
/* Drop only the rules and dir-merge registrations this file appended,
|
||||
leaving the caller's earlier content untouched. */
|
||||
for (int i = rules_before; i < list->count; i++)
|
||||
filter_rule_free(list->items[i]);
|
||||
list->count = rules_before;
|
||||
for (int i = dir_merges_before; i < list->dir_merge_count; i++)
|
||||
free(list->dir_merge_names[i]);
|
||||
list->dir_merge_count = dir_merges_before;
|
||||
filter_file_rollback(list, rules_before, dir_merges_before);
|
||||
return false;
|
||||
}
|
||||
for (int i = rules_before; i < list->count; i++) {
|
||||
if (!set_rule_owner(list->items[i], owner_rel)) {
|
||||
filter_set_error(err, err_size, "memory allocation failed");
|
||||
filter_file_rollback(list, rules_before, dir_merges_before);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
+4
-4
@@ -409,10 +409,10 @@ bool fake_super_restore_fd(int fd, FileAttrPolicy policy) {
|
||||
(void)ul_gid;
|
||||
/* Mode is applied only when the per-attribute policy asks for it, through the
|
||||
SAME shared helper the normal metadata path uses (metadata_mode_for_policy):
|
||||
group/other write bits are never granted, so a recorded source mode of 0666
|
||||
restores as 0644 — identical to a non-fake-super --preserve run, never a
|
||||
privilege-granting regression — and the -E rule derives exec bits from the
|
||||
destination's read bits exactly like file_restore_metadata_fd. */
|
||||
under --perms the recorded source mode is copied exactly, including
|
||||
group/other write and setuid/setgid/sticky bits (rsync parity), and the -E
|
||||
rule derives exec bits from the destination's read bits exactly like
|
||||
file_restore_metadata_fd. */
|
||||
if (policy.perms || policy.executability) {
|
||||
struct stat cur;
|
||||
mode_t want = 0;
|
||||
|
||||
+3
-2
@@ -105,8 +105,9 @@ void fake_super_store_fd(int fd, uint32_t uid, uint32_t gid, uint32_t mode, int6
|
||||
* absence of the xattr or a malformed record is a silent no-op that never fails
|
||||
* the transfer. The MODE leg is applied only when policy.perms||policy.
|
||||
* executability and the MTIME leg only when policy.times, so the fake-super
|
||||
* replay cannot bypass the per-attribute split; the mode is sanitized exactly
|
||||
* like the normal metadata path (group/other write bits never granted).
|
||||
* replay cannot bypass the per-attribute split; the mode follows the normal
|
||||
* metadata path exactly (under --perms the source mode is copied verbatim,
|
||||
* special and group/other write bits included).
|
||||
* Returns true when the xattr was present and parsed. */
|
||||
bool fake_super_restore_fd(int fd, FileAttrPolicy policy);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user