fix: address c-review for -R/--dirs/--mkpath row
B1: destination-root existence/creation mishandled two legitimate forms. file_directory_exists_secure/file_ensure_directory_secure now normalize a trailing-slash destination (so the last component is never an empty leaf) and treat a destination equal to the already-open authorized root as present (no spurious <root>/<basename> nested dir with --mkpath). Confinement and O_NOFOLLOW probing are unchanged. Regression integration tests: existing dest with trailing slash works with and without --mkpath; dest == authorized root works and creates no stray nested dir. W1: chunk serialize/deserialize round-trip unit test for a directory entry mixed with a regular file, with and without metadata; --dirs coverage under -s and -s -m. W2: --list-only and --dry-run now print file_wire_path() so all outputs show the -R transformed relative name, matching -i/--out-format. W3: RSYNC_COMPAT -d/--dirs note: dirs are created immediately under --delay-updates (only regular files are staged). W4: --dirs generator flushes chunks by element count too, so a long --files-from list of empty directories cannot exceed the per-chunk file cap. N1: STATUS_MKDIR added to status_to_string. N2: removed dead TestMkpath._transfer. N3: removed redundant --no-implied-dirs OPTION_TABLE row. N4: integration tests: --dirs --delete keeps the just-created empty dir (and deletes extras); a listed dir colliding with a regular file at the dest fails cleanly.
This commit is contained in:
+1
-1
@@ -74,7 +74,7 @@ This document maps rsync's full feature set to FastSync's current implementation
|
||||
| `-r`, `--recursive` | Recurse into directories | ✅ Implemented | Default behavior |
|
||||
| `-R`, `--relative` | Use relative path names | ✅ Implemented | Meaningful together with `--files-from` (FastSync's default full-tree scan always mirrors the full source argument path below the destination root, so -R does not change it). With `-R` + `--files-from` each listed entry is transmitted under its bare relative destination path: an entry `sub/x.txt` lands at `<dest>/sub/x.txt` (its leading components preserved) instead of under the `<dest>/<full source path>` mirror. Only the path sent on the wire changes; the client still reads the absolute source path, and the delete manifest derives from the sent (relative) paths so `--delete` and `--remove-source-files` stay consistent in both layouts. Works single-threaded and under `-m` (including chunk serialization) |
|
||||
| `--no-implied-dirs` | Don't send implied dirs with -R | ✅ Implemented | Client-side, meaningful only with `-R` + `--files-from`. rsync would normally create the ancestor directories implied by a listed file so it can be written; with `--no-implied-dirs` a listed file whose parent directory is not itself (or via an ancestor) explicitly listed cannot be placed, and FastSync fails the whole run up front with a clear error (`--no-implied-dirs: cannot place file '...': parent directory '...' is not explicitly listed`). Listing the directory (or an ancestor of it, or the whole tree `.`) permits the file. In every other mode the option has no effect. FastSync has no per-entry skip channel, so the rsync "omit the file" case is surfaced as a hard pre-transfer error |
|
||||
| `-d`, `--dirs`, `--old-dirs`, `--old-d` | Transfer dirs without recursing | ✅ Implemented | `-d <dir>` transmits an explicit directory entry for the source-root directory, so the destination mirror is created empty and nothing is descended into. With `--files-from` exactly the listed items are transferred: a listed directory is created empty (no descent) and a listed file is transferred with its content; the dest layout follows the same -R rules as plain files. A new wire frame (`STATUS_MKDIR`) carries each directory entry (path only); the receiver creates it with the same confined mkdir-parent semantics as regular writes, in single-threaded and `-m` receivers (chunk serialization carries a per-entry type marker). Directory entries appear in the delete manifest so `--delete` prunes correctly. FastSync divergences: directory mtimes/modes are not transmitted, filter/`--exclude` rules are not re-applied to the listed dirs mode (there is no descent during which they would apply), and `-d` never creates the intermediate directories between the destination root and a listed file beyond the usual on-demand parent creation |
|
||||
| `-d`, `--dirs`, `--old-dirs`, `--old-d` | Transfer dirs without recursing | ✅ Implemented | `-d <dir>` transmits an explicit directory entry for the source-root directory, so the destination mirror is created empty and nothing is descended into. With `--files-from` exactly the listed items are transferred: a listed directory is created empty (no descent) and a listed file is transferred with its content; the dest layout follows the same -R rules as plain files. A new wire frame (`STATUS_MKDIR`) carries each directory entry (path only); the receiver creates it with the same confined mkdir-parent semantics as regular writes, in single-threaded and `-m` receivers (chunk serialization carries a per-entry type marker). Directory entries appear in the delete manifest so `--delete` prunes correctly. FastSync divergences: directory mtimes/modes are not transmitted, filter/`--exclude` rules are not re-applied to the listed dirs mode (there is no descent during which they would apply), and `-d` never creates the intermediate directories between the destination root and a listed file beyond the usual on-demand parent creation. Under `--delay-updates` only regular files are staged: directory entries are created immediately, so a delayed run that fails part way can leave the already-created empty directories behind (matching rsync, which also creates directories as it processes the file list and only delays regular-file data) |
|
||||
| `--mkpath` | Create missing path components | ✅ Implemented | Wire option (client → server). At connection start the server creates the client's destination root directory (and any missing leading components below its own authorized root) when `--mkpath` is set, failing the connection cleanly if it cannot. Without `--mkpath` a destination root that does not exist yet is rejected up front (rsync semantics), so the flag is the only way to transfer into a not-yet-created destination directory. Creation is confined by the same secure mkdir walk as file writes (`O_NOFOLLOW`, no `..`) |
|
||||
|
||||
## 5. Transfer Modifications
|
||||
|
||||
@@ -429,7 +429,6 @@ static const OptionEntry OPTION_TABLE[] = {
|
||||
{"--old-dirs", NULL, OPT_FLAG, offsetof(Config, dirs)},
|
||||
{"--old-d", NULL, OPT_FLAG, offsetof(Config, dirs)},
|
||||
{"--relative", "-R", OPT_FLAG, offsetof(Config, relative)},
|
||||
{"--no-implied-dirs", NULL, OPT_FLAG, offsetof(Config, no_implied_dirs)},
|
||||
{"--mkpath", NULL, OPT_FLAG, offsetof(Config, mkpath)},
|
||||
{"--delete-during", "--del", OPT_UNSUPPORTED, 0},
|
||||
|
||||
|
||||
@@ -435,7 +435,8 @@ static int send_dry_run_manifest(const Config* config) {
|
||||
while ((chunk = directory_scanner_next(scanner)) != NULL) {
|
||||
for (int i = 0; i < chunk->element_count; i++) {
|
||||
if (!config->quiet) {
|
||||
char* escaped_path = output_escape(chunk->items[i]->path, config->eight_bit_output);
|
||||
char* escaped_path =
|
||||
output_escape(file_wire_path(chunk->items[i]), config->eight_bit_output);
|
||||
if (!escaped_path) {
|
||||
chunk_destroy(chunk);
|
||||
directory_scanner_destroy(scanner);
|
||||
@@ -529,7 +530,7 @@ static int send_list_only(const Config* config) {
|
||||
entries = grown;
|
||||
capacity = new_capacity;
|
||||
}
|
||||
char* path = str_dup(f->path);
|
||||
char* path = str_dup(file_wire_path(f));
|
||||
if (!path) {
|
||||
oom = true;
|
||||
break;
|
||||
|
||||
@@ -465,6 +465,11 @@ static int open_next_directory(DirectoryScanner* scanner) {
|
||||
listed regular files are transferred as files; nothing else is scanned, so
|
||||
no descent into a listed directory can happen. */
|
||||
|
||||
/* Directory entries carry no payload, so the dirs generator also bounds every
|
||||
chunk by element count; chunk_deserialize refuses more than this many files
|
||||
per chunk (see MAX_FILES_PER_CHUNK in chunk.c). */
|
||||
#define DIRS_CHUNK_MAX_FILES 65536U
|
||||
|
||||
/* Build the File for the transfer root directory itself (the `-d <dir>` and
|
||||
* "." cases). */
|
||||
static File* dirs_root_dir_file(DirectoryScanner* scanner) {
|
||||
@@ -618,6 +623,11 @@ static Chunk* directory_scanner_next_dirs(DirectoryScanner* scanner) {
|
||||
return NULL;
|
||||
}
|
||||
scanner->dirs_batch_size += file->data ? file->data->size : 0;
|
||||
/* Empty directory entries carry no bytes, so a large --dirs --files-from
|
||||
list must also be bounded by element count (the chunk deserializer caps
|
||||
the number of files per chunk). */
|
||||
if (scanner->dirs_batch->size >= (int)DIRS_CHUNK_MAX_FILES)
|
||||
return dirs_flush_batch(scanner);
|
||||
}
|
||||
return dirs_flush_batch(scanner);
|
||||
}
|
||||
|
||||
+47
-3
@@ -308,9 +308,41 @@ int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs)
|
||||
return fd;
|
||||
}
|
||||
|
||||
/* Normalized copy of a directory path: leading '/' kept, trailing '/' removed
|
||||
* ("/" and "//" both collapse to "/"). A trailing slash otherwise makes the
|
||||
* last path component empty, so probing that empty leaf below its parent
|
||||
* always fails. */
|
||||
static char* normalize_directory_path(const char* path) {
|
||||
if (!path)
|
||||
return NULL;
|
||||
size_t len = strlen(path);
|
||||
while (len > 1 && path[len - 1] == '/')
|
||||
len--;
|
||||
char* norm = malloc(len + 1);
|
||||
if (!norm)
|
||||
return NULL;
|
||||
memcpy(norm, path, len);
|
||||
norm[len] = '\0';
|
||||
return norm;
|
||||
}
|
||||
|
||||
bool file_ensure_directory_secure(const char* path) {
|
||||
if (!path)
|
||||
return false;
|
||||
char* norm = normalize_directory_path(path);
|
||||
if (!norm)
|
||||
return false;
|
||||
/* The authorized root is already an open directory, and the filesystem root
|
||||
is always present: there is no final component left to create for them. */
|
||||
bool root_is_open =
|
||||
authorized_root_fd >= 0 && authorized_root_path && strcmp(norm, authorized_root_path) == 0;
|
||||
if (root_is_open || strcmp(norm, "/") == 0) {
|
||||
free(norm);
|
||||
return true;
|
||||
}
|
||||
char* leaf = NULL;
|
||||
int parent_fd = file_open_secure_parent(path, &leaf, true);
|
||||
int parent_fd = file_open_secure_parent(norm, &leaf, true);
|
||||
free(norm);
|
||||
if (parent_fd < 0)
|
||||
return false;
|
||||
|
||||
@@ -329,12 +361,24 @@ bool file_ensure_directory_secure(const char* path) {
|
||||
|
||||
/* True when `path` resolves to an existing directory below the authorized root
|
||||
* (never creating anything). Used by the server to decide whether a client's
|
||||
* destination root already exists. */
|
||||
* destination root already exists. A trailing slash on `path` and a destination
|
||||
* equal to the authorized root itself are normalized/handled here so both
|
||||
* previously-working destination forms keep working. */
|
||||
bool file_directory_exists_secure(const char* path) {
|
||||
if (!path)
|
||||
return false;
|
||||
char* norm = normalize_directory_path(path);
|
||||
if (!norm)
|
||||
return false;
|
||||
bool root_is_open =
|
||||
authorized_root_fd >= 0 && authorized_root_path && strcmp(norm, authorized_root_path) == 0;
|
||||
if (root_is_open || strcmp(norm, "/") == 0) {
|
||||
free(norm);
|
||||
return true;
|
||||
}
|
||||
char* leaf = NULL;
|
||||
int parent_fd = file_open_secure_parent(path, &leaf, false);
|
||||
int parent_fd = file_open_secure_parent(norm, &leaf, false);
|
||||
free(norm);
|
||||
if (parent_fd < 0)
|
||||
return false;
|
||||
int dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
|
||||
|
||||
@@ -385,6 +385,8 @@ static const char* status_to_string(Status status) {
|
||||
return "ABORT";
|
||||
case STATUS_CHECK_BATCH:
|
||||
return "CHECK_BATCH";
|
||||
case STATUS_MKDIR:
|
||||
return "MKDIR";
|
||||
default:
|
||||
return "UNKNOWN";
|
||||
}
|
||||
|
||||
@@ -1842,20 +1842,67 @@ class TestDirs:
|
||||
assert not os.path.exists(os.path.join(received, "sub")), \
|
||||
"unlisted subtree appeared"
|
||||
|
||||
@pytest.mark.parametrize("mt", [False, True])
|
||||
def test_dirs_chunk_serialization(self, shared_server, mt):
|
||||
"""--dirs entries survive the chunk-serialization wire path (type
|
||||
marker round-trips); a listed dir lands empty and a listed file lands
|
||||
with content, with no protocol desync under -s -m."""
|
||||
source = self._make()
|
||||
dest = os.path.join(TEST_DATA_DIR, "dirs_s_dst")
|
||||
clean_dir(dest)
|
||||
lst = _write_rel_list(b"dir1\nsub/x.txt\n")
|
||||
flags = ["--files-from", lst, "--dirs", "-R", "-s"] + (["-m"] if mt else [])
|
||||
result, _ = run_client(source, dest, flags=flags, port=shared_server.port)
|
||||
assert result.returncode == 0, f"dirs -s sync failed: {result.stderr[:200]}"
|
||||
assert os.path.isdir(os.path.join(dest, "dir1")), "listed dir was not created"
|
||||
assert not os.path.exists(os.path.join(dest, "dir1", "keep.txt")), \
|
||||
"--dirs must not descend into a listed directory"
|
||||
assert _read_file(os.path.join(dest, "sub", "x.txt")) == b"x\n", \
|
||||
"listed file content was not transferred"
|
||||
|
||||
def test_dirs_delete_keeps_transferred_empty_dir(self):
|
||||
"""Directory entries appear in the delete manifest, so the empty dir a
|
||||
--dirs run just created is not pruned as an extra by --delete."""
|
||||
source = self._make()
|
||||
dest = os.path.join(TEST_DATA_DIR, "dirs_del_dst")
|
||||
clean_dir(dest)
|
||||
extra = os.path.join(dest, "extra.txt")
|
||||
with open(extra, "wb") as fh:
|
||||
fh.write(b"delete me")
|
||||
with ServerManager() as server:
|
||||
server.start(extra_args=["--allow-delete"])
|
||||
result, _ = run_client(source, dest, flags=["--dirs", "--delete"],
|
||||
port=server.port)
|
||||
assert result.returncode == 0, f"--dirs --delete sync failed: {result.stderr[:200]}"
|
||||
assert not os.path.exists(extra), "--delete did not remove the extra file"
|
||||
mirror = get_dest_received_dir(dest, source)
|
||||
assert os.path.isdir(mirror), "transferred empty dir was pruned as an extra"
|
||||
files = []
|
||||
for root, _dirs, names in os.walk(mirror):
|
||||
files.extend(os.path.relpath(os.path.join(root, n), mirror) for n in names)
|
||||
assert files == [], f"--dirs descended into contents: {files}"
|
||||
|
||||
def test_dirs_listed_dir_colliding_with_file_fails(self, shared_server):
|
||||
"""A listed directory that already exists as a regular file at the
|
||||
destination fails the transfer cleanly instead of clobbering the file."""
|
||||
source = self._make()
|
||||
dest = os.path.join(TEST_DATA_DIR, "dirs_coll_dst")
|
||||
clean_dir(dest)
|
||||
blocker = os.path.join(dest, "dir1")
|
||||
with open(blocker, "wb") as fh:
|
||||
fh.write(b"blocking file")
|
||||
lst = _write_rel_list(b"dir1\n")
|
||||
result, _ = run_client(source, dest, flags=["--files-from", lst, "--dirs", "-R"],
|
||||
port=shared_server.port)
|
||||
assert result.returncode != 0, "dir entry over an existing file did not fail"
|
||||
assert os.path.isfile(blocker), "blocking regular file was clobbered"
|
||||
|
||||
|
||||
class TestMkpath:
|
||||
"""--mkpath tells the server to create the destination root directory (and
|
||||
missing leading components) when it does not exist yet; without it a missing
|
||||
destination root fails the transfer."""
|
||||
|
||||
def _transfer(self, dest, mt, mkpath):
|
||||
source = _make_relative_source("mkpath_src")
|
||||
flags = ["--mkpath"] if mkpath else []
|
||||
if mt:
|
||||
flags += ["-m"]
|
||||
result, _ = run_client(source, dest, flags=flags, port=self.server.port)
|
||||
return result
|
||||
|
||||
@pytest.mark.parametrize("mt", [False, True])
|
||||
def test_missing_root_fails_without_mkpath(self, mt):
|
||||
source = _make_relative_source("mkpath_fail_src")
|
||||
@@ -1882,6 +1929,43 @@ class TestMkpath:
|
||||
assert _read_file(os.path.join(received, "sub", "x.txt")) == b"x\n", \
|
||||
"file not transferred into the --mkpath-created root"
|
||||
|
||||
@pytest.mark.parametrize("mkpath", [False, True])
|
||||
def test_existing_dest_with_trailing_slash(self, shared_server, mkpath):
|
||||
"""A destination root written with a trailing slash must keep working:
|
||||
an existing root is accepted both with and without --mkpath."""
|
||||
source = _make_relative_source("mkpath_trail_src")
|
||||
dest = os.path.join(TEST_DATA_DIR, "mkpath_trail_dst")
|
||||
clean_dir(dest)
|
||||
dest_slash = dest + "/"
|
||||
flags = ["--mkpath"] if mkpath else []
|
||||
result, _ = run_client(source, dest_slash, flags=flags, port=shared_server.port)
|
||||
assert result.returncode == 0, \
|
||||
f"trailing-slash dest sync (mkpath={mkpath}) failed: {result.stderr[:200]}"
|
||||
received = get_dest_received_dir(dest, source)
|
||||
assert _read_file(os.path.join(received, "sub", "x.txt")) == b"x\n", \
|
||||
"file not transferred into the trailing-slash destination root"
|
||||
|
||||
@pytest.mark.parametrize("mkpath", [False, True])
|
||||
def test_dest_equal_authorized_root(self, mkpath):
|
||||
"""A destination that is exactly the server's authorized root works
|
||||
without --mkpath, and with --mkpath creates no stray <root>/<basename>
|
||||
nested directory."""
|
||||
root = os.path.join(TEST_DATA_DIR, "mkpath_eq_root")
|
||||
clean_dir(root)
|
||||
source = _make_relative_source("mkpath_eq_src")
|
||||
with ServerManager() as server:
|
||||
server.start(extra_args=["--destination-root", root])
|
||||
flags = ["--mkpath"] if mkpath else []
|
||||
result, _ = run_client(source, root, flags=flags, port=server.port)
|
||||
assert result.returncode == 0, \
|
||||
f"dest==authorized-root sync (mkpath={mkpath}) failed: {result.stderr[:200]}"
|
||||
received = get_dest_received_dir(root, source)
|
||||
assert _read_file(os.path.join(received, "sub", "x.txt")) == b"x\n", \
|
||||
"file not transferred when the dest equals the authorized root"
|
||||
basename = os.path.basename(root.rstrip(os.sep))
|
||||
assert not os.path.exists(os.path.join(root, basename)), \
|
||||
"--mkpath created a spurious nested <root>/<basename> directory"
|
||||
|
||||
|
||||
class TestFilters:
|
||||
"""--filter/-C/-F rule layer: excludes prune, ordering is first-match-wins,
|
||||
|
||||
@@ -89,7 +89,78 @@ static void test_chunk_operations() {
|
||||
unlink(path2);
|
||||
}
|
||||
|
||||
/* A chunk mixing a regular file and an explicit directory entry (--dirs, with
|
||||
* or without metadata) must round-trip through serialize/deserialize with the
|
||||
* is_dir flag and the entry type marker preserved. */
|
||||
static void test_chunk_dir_entry_roundtrip() {
|
||||
const char* file_path = "temp_chunk_dir_file.txt";
|
||||
const char* dir_path = "temp_chunk_dir_entry";
|
||||
const char* content = "regular file payload";
|
||||
|
||||
/* A failed earlier run can leave artifacts behind; start clean. */
|
||||
rmdir(dir_path);
|
||||
unlink(file_path);
|
||||
|
||||
file_write_to_disk(file_path, content, strlen(content), false, false);
|
||||
EXPECT_EQ_INT(mkdir(dir_path, 0755), 0);
|
||||
|
||||
for (int use_metadata = 0; use_metadata <= 1; use_metadata++) {
|
||||
struct stat st;
|
||||
EXPECT_EQ_INT(stat(file_path, &st), 0);
|
||||
|
||||
File* reg = file_create(file_path);
|
||||
EXPECT_NOT_NULL(reg);
|
||||
reg->data->size = (unsigned long long)st.st_size;
|
||||
EXPECT_TRUE(file_load_data(reg));
|
||||
|
||||
File* dir = file_create(dir_path);
|
||||
EXPECT_NOT_NULL(dir);
|
||||
dir->is_dir = true;
|
||||
|
||||
if (use_metadata) {
|
||||
reg->metadata = file_metadata_create(&st);
|
||||
EXPECT_NOT_NULL(reg->metadata);
|
||||
struct stat dst;
|
||||
EXPECT_EQ_INT(stat(dir_path, &dst), 0);
|
||||
dir->metadata = file_metadata_create(&dst);
|
||||
EXPECT_NOT_NULL(dir->metadata);
|
||||
}
|
||||
|
||||
File* files[2] = {reg, dir};
|
||||
Chunk* chunk = chunk_create(files, 2);
|
||||
EXPECT_NOT_NULL(chunk);
|
||||
|
||||
Data* serialized = chunk_serialize(chunk, use_metadata != 0);
|
||||
EXPECT_NOT_NULL(serialized);
|
||||
Chunk* deserialized = chunk_deserialize(serialized, use_metadata != 0);
|
||||
EXPECT_NOT_NULL(deserialized);
|
||||
EXPECT_EQ_INT(deserialized->element_count, 2);
|
||||
EXPECT_FALSE(deserialized->items[0]->is_dir);
|
||||
EXPECT_EQ_STR(deserialized->items[0]->path, file_path);
|
||||
EXPECT_EQ_INT((int)deserialized->items[0]->data->size, (int)strlen(content));
|
||||
EXPECT_EQ_INT(memcmp(deserialized->items[0]->data->data, content, strlen(content)), 0);
|
||||
EXPECT_TRUE(deserialized->items[1]->is_dir);
|
||||
EXPECT_EQ_STR(deserialized->items[1]->path, dir_path);
|
||||
EXPECT_EQ_INT((int)deserialized->items[1]->data->size, 0);
|
||||
if (use_metadata) {
|
||||
EXPECT_NOT_NULL(deserialized->items[0]->metadata);
|
||||
EXPECT_NOT_NULL(deserialized->items[1]->metadata);
|
||||
} else {
|
||||
EXPECT_NULL(deserialized->items[0]->metadata);
|
||||
EXPECT_NULL(deserialized->items[1]->metadata);
|
||||
}
|
||||
|
||||
data_destroy(serialized);
|
||||
chunk_destroy(deserialized);
|
||||
chunk_destroy(chunk); /* frees reg and dir */
|
||||
}
|
||||
|
||||
unlink(file_path);
|
||||
rmdir(dir_path);
|
||||
}
|
||||
|
||||
void test_chunk() {
|
||||
test_file_operations();
|
||||
test_chunk_operations();
|
||||
test_chunk_dir_entry_roundtrip();
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user