From 5e67a721a46502bd29f7afc7d84fc7f690647ac0 Mon Sep 17 00:00:00 2001 From: TapTap Date: Thu, 3 Sep 2026 17:07:02 +0200 Subject: [PATCH 1/2] feat: add rsync-compatible update option --- src/client/client_cli.c | 3 +++ src/client/usage.c | 1 + src/shared/file_receive.c | 25 ++++++++++++++-------- tests/integration/test_features.py | 34 ++++++++++++++++++++++++++++++ tests/test_client_cli.c | 17 +++++++++++++-- 5 files changed, 69 insertions(+), 11 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 191cc07..1a98aae 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -146,6 +146,7 @@ static const OptionEntry OPTION_TABLE[] = { {"--backup", NULL, OPT_FLAG, offsetof(Config, backup)}, {"--stats", NULL, OPT_FLAG, offsetof(Config, stats)}, {"--partial", NULL, OPT_FLAG, offsetof(Config, partial)}, + {"--update", "-u", OPT_FLAG, offsetof(Config, update)}, {"--links", "-l", OPT_FLAG, offsetof(Config, follow_symlinks)}, {"--copy-links", NULL, OPT_FLAG, offsetof(Config, copy_links)}, {"--safe-links", NULL, OPT_FLAG, offsetof(Config, safe_links)}, @@ -189,6 +190,8 @@ static int apply_table_option(Config* config, const OptionEntry* entry, const ch switch (entry->kind) { case OPT_FLAG: *(bool*)field = true; + if (entry->offset == offsetof(Config, update)) + config->use_metadata = true; return 0; case OPT_STRING: return set_string_option((char**)field, value, entry->name); diff --git a/src/client/usage.c b/src/client/usage.c index 2f800af..d16718c 100644 --- a/src/client/usage.c +++ b/src/client/usage.c @@ -28,6 +28,7 @@ void print_usage(void) { printf(" --max-size Skip files larger than n bytes\n"); printf(" --min-size Skip files smaller than n bytes\n"); printf(" --incremental Skip files unchanged since last transfer\n"); + printf(" -u, --update Skip files newer than the source on receiver\n"); printf(" --delta Delta transfer for changed files (requires --incremental)\n"); printf(" --delta-block Delta block size in bytes (default: %d)\n", DELTA_BLOCK_SIZE_DEFAULT); diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index a610d67..93edb1e 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -21,6 +21,17 @@ #define MAX_SERVER_DELETE_COUNT 100000U #define MAX_FILE_DATA_SIZE MAX_RECEIVE_FILE_SIZE +static bool destination_is_newer(const char* path, const FileMetadata* source_metadata) { + struct stat destination_stat; + if (!source_metadata || !file_stat_secure(path, &destination_stat)) + return false; + time_t destination_sec = destination_stat.st_mtim.tv_sec; + long destination_nsec = destination_stat.st_mtim.tv_nsec; + return destination_sec > source_metadata->mtime_sec || + (destination_sec == source_metadata->mtime_sec && + destination_nsec > source_metadata->mtime_nsec); +} + bool file_save_to_disk(const char* root_directory, const File* file, const Config* config) { bool backup_enabled = config && config->backup; bool inplace = config && config->inplace; @@ -62,15 +73,11 @@ bool file_save_to_disk(const char* root_directory, const File* file, const Confi } /* --update is receiver-side policy: never replace a newer destination. */ - if (config && config->update) { - struct stat destination_stat; - if (file_stat_secure(disk_path, &destination_stat) && file->metadata && - destination_stat.st_mtime > file->metadata->mtime_sec) { - free(confined_backup); - free(confined_partial); - free(disk_path); - return true; - } + if (config && config->update && destination_is_newer(disk_path, file->metadata)) { + free(confined_backup); + free(confined_partial); + free(disk_path); + return true; } if (backup_enabled) { diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 312add4..60d4427 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -214,6 +214,40 @@ class TestIncremental: assert f.read() == b"hello world\n" +class TestUpdate: + def test_update_skips_older_destination_and_allows_equal_or_newer_source(self, shared_server): + clean_dir(DEST_DIR) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + + received = get_dest_received_dir(DEST_DIR, SOURCE_DIR) + source_file = os.path.join(SOURCE_DIR, "small.txt") + received_file = os.path.join(received, "small.txt") + source_stat = os.stat(source_file) + + with open(received_file, "wb") as f: + f.write(b"newer destination\n") + os.utime(received_file, (source_stat.st_mtime + 10, source_stat.st_mtime + 10)) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + with open(received_file, "rb") as f: + assert f.read() == b"newer destination\n" + + os.utime(received_file, (source_stat.st_atime, source_stat.st_mtime)) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + with open(received_file, "rb") as f: + assert f.read() == b"hello world\n" + + with open(received_file, "wb") as f: + f.write(b"older destination\n") + os.utime(received_file, (source_stat.st_atime, source_stat.st_mtime - 10)) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + with open(received_file, "rb") as f: + assert f.read() == b"hello world\n" + + class TestDelete: def test_delete_removes_extra_files(self, shared_server): clean_dir(DEST_DIR) diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 656fd40..03fb769 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -282,8 +282,6 @@ static void test_parse_args_rejects_unimplemented_options() { "--list-only", "-h", "--human-readable", - "-u", - "--update", "--append", "--append-verify", "--delete-excluded", @@ -323,6 +321,20 @@ static void test_parse_args_rejects_unimplemented_options() { } } +static void test_parse_args_update() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "-u", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + + EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), 0); + EXPECT_TRUE(cfg->update); + EXPECT_TRUE(cfg->use_metadata); + EXPECT_EQ_INT(positional_count, 2); + + config_delete(cfg); +} + /* Test parse_args with --archive flag */ static void test_parse_args_archive() { Config* cfg = config_create(); @@ -359,5 +371,6 @@ void test_client_cli() { test_parse_args_valid_compression_level(); test_parse_args_unknown_option(); test_parse_args_rejects_unimplemented_options(); + test_parse_args_update(); test_parse_args_archive(); } From 68aaf89da75bc41a615a63de8150ae76a6c80cf2 Mon Sep 17 00:00:00 2001 From: TapTap Date: Thu, 3 Sep 2026 21:59:55 +0200 Subject: [PATCH 2/2] fix: address update option review findings --- src/shared/file.c | 69 +++++++++++++++++++++++++----- src/shared/file.h | 6 +++ src/shared/file_receive.c | 23 ++++------ tests/integration/test_features.py | 27 ++++++++++-- 4 files changed, 96 insertions(+), 29 deletions(-) diff --git a/src/shared/file.c b/src/shared/file.c index 9514e87..c7e357c 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -183,15 +183,29 @@ bool file_stat_secure(const char* path, struct stat* st) { 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_NONBLOCK | O_CLOEXEC | O_NOFOLLOW); - bool exists = fd >= 0 && fstat(fd, st) == 0 && S_ISREG(st->st_mode); - if (fd >= 0) - close(fd); + bool exists = fstatat(parent_fd, leaf, st, AT_SYMLINK_NOFOLLOW) == 0 && S_ISREG(st->st_mode); close(parent_fd); free(leaf); return exists; } +static bool stat_is_newer(const struct stat* st, const FileMetadata* metadata) { + if (!st || !metadata) + return false; +#ifdef __linux__ + long mtime_nsec = st->st_mtim.tv_nsec; +#else + long mtime_nsec = 0; +#endif + return st->st_mtime > metadata->mtime_sec || + (st->st_mtime == metadata->mtime_sec && mtime_nsec > metadata->mtime_nsec); +} + +bool file_destination_is_newer_secure(const char* path, const FileMetadata* metadata) { + struct stat st; + return file_stat_secure(path, &st) && stat_is_newer(&st, metadata); +} + int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) { char* copy = str_dup(path); if (!copy) @@ -302,8 +316,9 @@ bool file_rename_secure(const char* old_path, const char* new_path) { return ok; } -bool file_to_disk_secure(const char* path, const void* data, unsigned long long data_size, - bool inplace, bool sparse, const FileMetadata* metadata) { +static bool file_to_disk_secure_impl(const char* path, const void* data, + unsigned long long data_size, bool inplace, bool sparse, + const FileMetadata* metadata, bool update) { char* leaf = NULL; int dirfd = file_open_secure_parent(path, &leaf, true); if (dirfd < 0) @@ -311,15 +326,37 @@ bool file_to_disk_secure(const char* path, const void* data, unsigned long long int fd = -1; bool ok = false; if (inplace) { - fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, 0644); + fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW, 0644); if (fd >= 0) { - if (!sparse || data_size == 0 || ftruncate(fd, (off_t)data_size) == 0) - ok = write_all(fd, data, data_size); - if (ok && metadata) - ok = file_restore_metadata_fd(fd, metadata); + struct stat destination_stat; + 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 (newer) { + ok = true; + } else { + if (ftruncate(fd, 0) == 0 && + (!sparse || data_size == 0 || ftruncate(fd, (off_t)data_size) == 0)) + ok = write_all(fd, data, data_size); + if (ok && metadata) + ok = file_restore_metadata_fd(fd, metadata); + } } } else { char tmp[NAME_MAX]; + if (update && metadata) { + /* This check protects the normal atomic path as far as possible. A + concurrent replacement can still occur before the final rename. */ + struct stat destination_stat; + if (fstatat(dirfd, leaf, &destination_stat, AT_SYMLINK_NOFOLLOW) == 0 && + S_ISREG(destination_stat.st_mode) && stat_is_newer(&destination_stat, metadata)) { + close(dirfd); + free(leaf); + return true; + } + } for (unsigned int i = 0; i < 100 && !ok; ++i) { snprintf(tmp, sizeof(tmp), ".%s.tmp.%ld.%u", leaf, (long)getpid(), i); fd = openat(dirfd, tmp, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC | O_NOFOLLOW, 0600); @@ -347,6 +384,16 @@ bool file_to_disk_secure(const char* path, const void* data, unsigned long long return ok; } +bool file_to_disk_secure(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, const FileMetadata* metadata) { + return file_to_disk_secure_impl(path, data, data_size, inplace, sparse, metadata, false); +} + +bool file_to_disk_secure_update(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, const FileMetadata* metadata) { + return file_to_disk_secure_impl(path, data, data_size, inplace, sparse, metadata, true); +} + bool file_write_to_disk(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse) { if (!path || (!data && data_size != 0) || has_path_traversal(path)) diff --git a/src/shared/file.h b/src/shared/file.h index 23dac26..67a471c 100644 --- a/src/shared/file.h +++ b/src/shared/file.h @@ -27,10 +27,16 @@ bool file_set_authorized_root(int fd, const char* canonical_path); /* Secure path/filesystem primitives (symlink-safe, O_NOFOLLOW, root-confined). */ bool file_path_exists_secure(const char* path); bool file_stat_secure(const char* path, struct stat* st); +bool file_destination_is_newer_secure(const char* path, const FileMetadata* metadata); int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs); bool file_ensure_directory_secure(const char* path); bool file_rename_secure(const char* old_path, const char* new_path); bool file_to_disk_secure(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse, const FileMetadata* metadata); +/* With update enabled, an existing newer destination is left untouched. The + check is descriptor-based for inplace writes; atomic replacement still has + an unavoidable final rename race without filesystem locking. */ +bool file_to_disk_secure_update(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, const FileMetadata* metadata); #endif diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index 93edb1e..a51f39d 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -21,17 +21,6 @@ #define MAX_SERVER_DELETE_COUNT 100000U #define MAX_FILE_DATA_SIZE MAX_RECEIVE_FILE_SIZE -static bool destination_is_newer(const char* path, const FileMetadata* source_metadata) { - struct stat destination_stat; - if (!source_metadata || !file_stat_secure(path, &destination_stat)) - return false; - time_t destination_sec = destination_stat.st_mtim.tv_sec; - long destination_nsec = destination_stat.st_mtim.tv_nsec; - return destination_sec > source_metadata->mtime_sec || - (destination_sec == source_metadata->mtime_sec && - destination_nsec > source_metadata->mtime_nsec); -} - bool file_save_to_disk(const char* root_directory, const File* file, const Config* config) { bool backup_enabled = config && config->backup; bool inplace = config && config->inplace; @@ -72,8 +61,9 @@ bool file_save_to_disk(const char* root_directory, const File* file, const Confi return false; } - /* --update is receiver-side policy: never replace a newer destination. */ - if (config && config->update && destination_is_newer(disk_path, file->metadata)) { + /* --update is receiver-side policy: never replace a newer destination. + The secure stat does not require read permission on the destination. */ + if (config && config->update && file_destination_is_newer_secure(disk_path, file->metadata)) { free(confined_backup); free(confined_partial); free(disk_path); @@ -110,8 +100,11 @@ bool file_save_to_disk(const char* root_directory, const File* file, const Confi } } - bool ok = file_to_disk_secure(disk_path, file->data->data, file->data->size, inplace, sparse, - file->metadata); + bool ok = config && config->update + ? file_to_disk_secure_update(disk_path, file->data->data, file->data->size, inplace, + sparse, file->metadata) + : file_to_disk_secure(disk_path, file->data->data, file->data->size, inplace, + sparse, file->metadata); free(parent_copy); free(backup_path); free(confined_backup); diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 60d4427..6dbb333 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -227,21 +227,42 @@ class TestUpdate: with open(received_file, "wb") as f: f.write(b"newer destination\n") - os.utime(received_file, (source_stat.st_mtime + 10, source_stat.st_mtime + 10)) + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns + 10_000_000_000)) result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) assert result.returncode == 0 with open(received_file, "rb") as f: assert f.read() == b"newer destination\n" - os.utime(received_file, (source_stat.st_atime, source_stat.st_mtime)) + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns)) result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) assert result.returncode == 0 with open(received_file, "rb") as f: assert f.read() == b"hello world\n" + def test_update_skips_unreadable_newer_destination(self, shared_server): + clean_dir(DEST_DIR) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + + received_file = os.path.join(get_dest_received_dir(DEST_DIR, SOURCE_DIR), "small.txt") + source_stat = os.stat(os.path.join(SOURCE_DIR, "small.txt")) + with open(received_file, "wb") as f: + f.write(b"protected destination\n") + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns + 10_000_000_000)) + original_mode = os.stat(received_file).st_mode + try: + os.chmod(received_file, 0) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + os.chmod(received_file, original_mode) + with open(received_file, "rb") as f: + assert f.read() == b"protected destination\n" + finally: + os.chmod(received_file, original_mode) + with open(received_file, "wb") as f: f.write(b"older destination\n") - os.utime(received_file, (source_stat.st_atime, source_stat.st_mtime - 10)) + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns - 10_000_000_000)) result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) assert result.returncode == 0 with open(received_file, "rb") as f: