diff --git a/src/client/client_send.c b/src/client/client_send.c index c307107..21b32b8 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -386,6 +386,19 @@ static int scan_directory_multithreaded(void* pipeline_context) { context->config->checksum); Chunk* current_chunk; + if (scanner == NULL) { + log_message(LOG_LEVEL_ERROR, "Failed to create parallel scanner"); + atomic_store(&context->cancelled, true); + cnd_broadcast(&context->condition_not_full_scanner); + cnd_broadcast(&context->condition_not_empty_scanner); + cnd_broadcast(&context->condition_not_full_loader); + cnd_broadcast(&context->condition_not_empty_loader); + mtx_lock(&context->mutex_scanner); + context->scanner_done = true; + cnd_broadcast(&context->condition_not_empty_scanner); + mtx_unlock(&context->mutex_scanner); + return thrd_error; + } while ((current_chunk = parallel_scanner_next(scanner)) != NULL) { if (context->config->use_delete) { mtx_lock(&context->mutex_scanner); @@ -397,13 +410,22 @@ static int scan_directory_multithreaded(void* pipeline_context) { if (!manifest_entry) { log_message(LOG_LEVEL_ERROR, "Failed to allocate manifest entry"); mtx_unlock(&context->mutex_scanner); - context->cancelled = true; + atomic_store(&context->cancelled, true); cnd_broadcast(&context->condition_not_full_scanner); cnd_broadcast(&context->condition_not_empty_scanner); parallel_scanner_destroy(scanner); return thrd_error; } - array_list_add(context->manifest, manifest_entry); + if (!array_list_add(context->manifest, manifest_entry)) { + free(manifest_entry); + atomic_store(&context->cancelled, true); + cnd_broadcast(&context->condition_not_full_scanner); + cnd_broadcast(&context->condition_not_empty_scanner); + mtx_unlock(&context->mutex_scanner); + chunk_destroy(current_chunk); + parallel_scanner_destroy(scanner); + return thrd_error; + } } mtx_unlock(&context->mutex_scanner); } @@ -412,7 +434,7 @@ static int scan_directory_multithreaded(void* pipeline_context) { &context->condition_not_empty_scanner, &context->condition_not_full_scanner, &context->cancelled)) { chunk_destroy(current_chunk); - context->cancelled = true; + atomic_store(&context->cancelled, true); cnd_broadcast(&context->condition_not_full_scanner); cnd_broadcast(&context->condition_not_empty_scanner); parallel_scanner_destroy(scanner); @@ -458,7 +480,7 @@ static int load_files_multithreaded(void* pipeline_context) { &context->condition_not_full_loader, &context->cancelled)) { chunk_destroy(chunk); - context->cancelled = true; + atomic_store(&context->cancelled, true); cnd_broadcast(&context->condition_not_full_loader); cnd_broadcast(&context->condition_not_empty_loader); return thrd_error; @@ -573,7 +595,15 @@ int send_files(Config* config) { client_delete(client); return 1; } - array_list_add(manifest, manifest_entry); + if (!array_list_add(manifest, manifest_entry)) { + free(manifest_entry); + chunk_destroy(current_chunk); + array_list_delete(manifest); + directory_scanner_destroy(scanner); + client_disconnect(client); + client_delete(client); + return 1; + } } } if (!config->use_sendfile) { @@ -685,7 +715,7 @@ int send_files_multithreaded(Config* config) { if (!scanner_created || !loader_created || !sender_created) { perror("Error creating threads.\n"); - context->cancelled = true; + atomic_store(&context->cancelled, true); context->scanner_done = true; context->loader_done = true; context->sender_done = true; diff --git a/src/server/server.c b/src/server/server.c index fe5f3f4..2cc98f0 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -57,8 +57,12 @@ int receive_files(Config* config, int fd) { goto next; if (file == NULL && !skipped) return -1; - if (config->save_to_disk) - file_save_to_disk(config->receive_root_directory, file, config); + if (config->save_to_disk && + !file_save_to_disk(config->receive_root_directory, file, config)) { + file_destroy(file); + send_status(fd, STATUS_ERROR); + return -1; + } file_destroy(file); } else if (status == STATUS_CHUNK) { Chunk* chunk = receive_chunk_data(fd, config); @@ -67,13 +71,19 @@ int receive_files(Config* config, int fd) { return -1; } for (int i = 0; i < chunk->element_count; i++) { - if (config->save_to_disk) - file_save_to_disk(config->receive_root_directory, chunk->items[i], config); + if (config->save_to_disk && + !file_save_to_disk(config->receive_root_directory, chunk->items[i], config)) { + chunk_destroy(chunk); + send_status(fd, STATUS_ERROR); + return -1; + } } chunk_destroy(chunk); } else if (status == STATUS_CHECK_BATCH) { int count; - if (!receive_int(fd, &count)) + /* Batch framing has no checksum field yet; never silently downgrade a + checksum-enabled transfer into mtime-only matching. */ + if (config->checksum || !receive_int(fd, &count) || count < 0 || count > MAX_MANIFEST_ENTRIES) return -1; for (int i = 0; i < count; i++) { char* check_path = receive_str(fd); @@ -106,8 +116,12 @@ int receive_files(Config* config, int fd) { send_status(fd, STATUS_ERROR); return -1; } - if (config->save_to_disk) - file_save_to_disk(config->receive_root_directory, file, config); + if (config->save_to_disk && + !file_save_to_disk(config->receive_root_directory, file, config)) { + file_destroy(file); + send_status(fd, STATUS_ERROR); + return -1; + } file_destroy(file); } next: @@ -193,7 +207,7 @@ void handler(int file_descriptor) { perror("Error creating Threads"); if (receiver_created) { mtx_lock(&context->mutex); - context->cancelled = true; + atomic_store(&context->cancelled, true); cnd_broadcast(&context->condition_not_full); cnd_broadcast(&context->condition_not_empty); mtx_unlock(&context->mutex); @@ -213,9 +227,12 @@ void handler(int file_descriptor) { thrd_join(writer, &writer_result); if (receiver_result == thrd_success && writer_result == thrd_success) send_status(file_descriptor, STATUS_OK); + else + send_status(file_descriptor, STATUS_ERROR); pipeline_context_receiver_destroy(context); } else { - receive_files(config, file_descriptor); + if (receive_files(config, file_descriptor) != 0) + log_message(LOG_LEVEL_ERROR, "Transfer failed"); config_delete(config); } close(file_descriptor); diff --git a/src/shared/file.c b/src/shared/file.c index 451f37c..c4419da 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -145,6 +145,14 @@ bool file_send_single_calls(File* file, int file_descriptor, bool use_metadata, return true; } +static bool to_disk_secure(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, FileMetadata* metadata); + +static bool path_is_within_root(const char* root, const char* path) { + size_t n = strlen(root); + return strncmp(root, path, n) == 0 && (path[n] == '\0' || path[n] == '/'); +} + bool file_save_to_disk(const char* root_directory, File* file, const Config* config) { bool backup_enabled = config && config->backup; bool inplace = config && config->inplace; @@ -152,15 +160,28 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con const char* backup_suffix = (config && config->suffix) ? config->suffix : "~"; const char* backup_dir = (config && config->backup_dir) ? config->backup_dir : NULL; const char* partial_dir = (config && config->partial_dir) ? config->partial_dir : NULL; + char *confined_backup = NULL, *confined_partial = NULL; if (has_path_traversal(file->path)) { log_message(LOG_LEVEL_ERROR, "Path traversal detected in file path: %s", file->path); return false; } + /* These options arrive from the client. They are names below the server + root, never independent filesystem roots. */ + if ((backup_dir && (backup_dir[0] == '/' || has_path_traversal(backup_dir))) || + (partial_dir && (partial_dir[0] == '/' || has_path_traversal(partial_dir)))) + return false; + if (backup_dir && !(confined_backup = path_cat(root_directory, backup_dir))) + return false; + if (partial_dir && !(confined_partial = path_cat(root_directory, partial_dir))) { + free(confined_backup); + return false; + } + char* resolved_root = NULL; const char* actual_root = - (partial_dir && config && config->partial) ? partial_dir : root_directory; + (partial_dir && config && config->partial) ? confined_partial : root_directory; resolved_root = realpath(actual_root, NULL); if (resolved_root == NULL) { if (mkdir_r(actual_root)) { @@ -169,11 +190,24 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con } if (resolved_root == NULL) { log_message(LOG_LEVEL_ERROR, "Failed to resolve destination root: %s", actual_root); + free(confined_backup); + free(confined_partial); return false; } + char* resolved_base = realpath(root_directory, NULL); + if (resolved_base == NULL || !path_is_within_root(resolved_base, resolved_root)) { + free(resolved_base); + free(confined_backup); + free(confined_partial); + free(resolved_root); + return false; + } + free(resolved_base); char* disk_path = path_cat(resolved_root, file->path); if (disk_path == NULL) { + free(confined_backup); + free(confined_partial); free(resolved_root); return false; } @@ -184,6 +218,8 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con if (stat(disk_path, &destination_stat) == 0 && file->metadata && destination_stat.st_mtime > file->metadata->mtime_sec) { free(resolved_root); + free(confined_backup); + free(confined_partial); free(disk_path); return true; } @@ -194,13 +230,16 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con if (stat(disk_path, &backup_stat) == 0) { char* backup_path = NULL; if (backup_dir) { - char* resolved_backup_dir = realpath(backup_dir, NULL); + char* resolved_backup_dir = realpath(confined_backup, NULL); if (!resolved_backup_dir) { - mkdir_r(backup_dir); - resolved_backup_dir = realpath(backup_dir, NULL); + mkdir_r(confined_backup); + resolved_backup_dir = realpath(confined_backup, NULL); } if (resolved_backup_dir) { - backup_path = path_cat(resolved_backup_dir, file->path); + char* backup_base = realpath(root_directory, NULL); + if (backup_base && path_is_within_root(backup_base, resolved_backup_dir)) + backup_path = path_cat(resolved_backup_dir, file->path); + free(backup_base); free(resolved_backup_dir); } } @@ -228,6 +267,8 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con char* dir_dup = str_dup(disk_path); if (!dir_dup) { + free(confined_backup); + free(confined_partial); free(resolved_root); free(disk_path); return false; @@ -235,6 +276,8 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con char* dir_str = dirname(dir_dup); if (!mkdir_r(dir_str)) { free(dir_dup); + free(confined_backup); + free(confined_partial); free(resolved_root); free(disk_path); return false; @@ -243,6 +286,8 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con free(dir_dup); if (resolved_dir == NULL) { log_message(LOG_LEVEL_ERROR, "Failed to resolve directory for: %s", disk_path); + free(confined_backup); + free(confined_partial); free(resolved_root); free(disk_path); return false; @@ -253,6 +298,8 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con (resolved_dir[root_len] != '\0' && resolved_dir[root_len] != '/')) { log_message(LOG_LEVEL_ERROR, "Path escape detected: %s is outside %s", disk_path, actual_root); free(resolved_dir); + free(confined_backup); + free(confined_partial); free(resolved_root); free(disk_path); return false; @@ -260,9 +307,10 @@ bool file_save_to_disk(const char* root_directory, File* file, const Config* con free(resolved_dir); free(resolved_root); - bool ok = to_disk(disk_path, file->data->data, file->data->size, inplace, sparse); - if (ok) - file_restore_metadata(disk_path, file->metadata); + bool ok = to_disk_secure(disk_path, file->data->data, file->data->size, inplace, sparse, + file->metadata); + free(confined_backup); + free(confined_partial); free(disk_path); return ok; } @@ -616,7 +664,7 @@ static bool write_all(int fd, const void* data, unsigned long long size) { } static bool to_disk_secure(const char* path, const void* data, unsigned long long data_size, - bool inplace, bool sparse) { + bool inplace, bool sparse, FileMetadata* metadata) { char* leaf = NULL; int dirfd = open_secure_parent(path, &leaf); if (dirfd < 0) @@ -628,6 +676,8 @@ static bool to_disk_secure(const char* path, const void* data, unsigned long lon 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) + file_restore_metadata_fd(fd, metadata); } } else { char tmp[NAME_MAX]; @@ -640,6 +690,8 @@ static bool to_disk_secure(const char* path, const void* data, unsigned long lon ok = ftruncate(fd, (off_t)data_size) == 0; if (ok || (!sparse || data_size == 0)) ok = write_all(fd, data, data_size); + if (ok && metadata) + file_restore_metadata_fd(fd, metadata); if (close(fd) != 0) ok = false; fd = -1; @@ -660,7 +712,7 @@ bool to_disk(const char* path, const void* data, unsigned long long data_size, b bool sparse) { if (!path || (!data && data_size != 0) || has_path_traversal(path)) return false; - return to_disk_secure(path, data, data_size, inplace, sparse); + return to_disk_secure(path, data, data_size, inplace, sparse, NULL); /* Kept below only as historical context; all writes use descriptor-relative operations. */ char* tmp_path = NULL; char* directory = NULL; @@ -867,18 +919,30 @@ int receive_manifest(int fd, const Config* config, int* next_status) { ArrayList* manifest = array_list_create(free); if (!manifest) return -1; + size_t manifest_bytes = 0; for (int i = 0; i < count; i++) { char* s = receive_str(fd); - if (!s || s[0] == '\0' || has_path_traversal(s) || !array_list_add(manifest, s)) { + size_t entry_size = s ? strlen(s) : 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(manifest, s)) { free(s); array_list_delete(manifest); return -1; } } + if (!receive_status(fd, next_status)) { + array_list_delete(manifest); + return -1; + } + /* Deletion is a commit operation: never perform it until the sender has + completed the manifest frame successfully. */ + if (*next_status != STATUS_FINISHED || !config->use_delete) { + array_list_delete(manifest); + return *next_status == STATUS_FINISHED ? 0 : -1; + } fprintf(stderr, "Deleting files not in manifest...\n"); delete_extras(config->receive_root_directory, manifest); array_list_delete(manifest); - if (!receive_status(fd, next_status)) - return -1; return 0; } diff --git a/src/shared/metadata.c b/src/shared/metadata.c index 7f246df..6502e37 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -187,3 +187,17 @@ void file_restore_metadata(const char* path, FileMetadata* metadata) { if (utimensat(AT_FDCWD, path, times, 0) != 0) log_message(LOG_LEVEL_WARNING, "Failed to set timestamps on %s: %s", path, strerror(errno)); } + +void file_restore_metadata_fd(int fd, FileMetadata* metadata) { + if (fd < 0 || metadata == NULL) + return; + if (fchmod(fd, metadata->mode & 07777 & ~(S_ISUID | S_ISGID)) != 0) + log_message(LOG_LEVEL_WARNING, "Failed to fchmod received file: %s", strerror(errno)); + if (fchown(fd, metadata->uid, metadata->gid) != 0 && errno != EPERM) + log_message(LOG_LEVEL_WARNING, "Failed to fchown received file: %s", strerror(errno)); + struct timespec times[2] = {{.tv_sec = metadata->mtime_sec, .tv_nsec = metadata->mtime_nsec}, + {.tv_sec = metadata->mtime_sec, .tv_nsec = metadata->mtime_nsec}}; + if (futimens(fd, times) != 0) + log_message(LOG_LEVEL_WARNING, "Failed to restore received file timestamps: %s", + strerror(errno)); +} diff --git a/src/shared/metadata.h b/src/shared/metadata.h index 28312f5..7654dfc 100644 --- a/src/shared/metadata.h +++ b/src/shared/metadata.h @@ -30,5 +30,6 @@ FileMetadata* metadata_from_buf(char** buf); bool metadata_send(int file_descriptor, FileMetadata* m); FileMetadata* metadata_receive(int file_descriptor, int* ok); void file_restore_metadata(const char* path, FileMetadata* metadata); +void file_restore_metadata_fd(int fd, FileMetadata* metadata); #endif diff --git a/src/shared/multiprocessing.c b/src/shared/multiprocessing.c index 9df6674..162add0 100644 --- a/src/shared/multiprocessing.c +++ b/src/shared/multiprocessing.c @@ -26,7 +26,7 @@ PipelineContextSender* pipeline_context_sender_create(Config* config, Queue* que context->manifest = NULL; context->progress_bytes = 0; context->sender_done = false; - context->cancelled = false; + atomic_init(&context->cancelled, false); int init = 0; if (mtx_init(&context->mutex_scanner, mtx_plain) != thrd_success) goto fail; @@ -97,7 +97,7 @@ PipelineContextReceiver* pipeline_context_receiver_create(Config* config, Queue* context->file_descriptor = file_descriptor; context->ssl = ssl; context->receiver_done = false; - context->cancelled = false; + atomic_init(&context->cancelled, false); int init = 0; if (mtx_init(&context->mutex, mtx_plain) != thrd_success) goto fail; @@ -154,7 +154,7 @@ static bool receive_chunk_enqueue(int file_descriptor, PipelineContextReceiver* static void receiver_thread_fail(PipelineContextReceiver* context) { mtx_lock(&context->mutex); - context->cancelled = true; + atomic_store(&context->cancelled, true); context->receiver_done = true; cnd_broadcast(&context->condition_not_empty); cnd_broadcast(&context->condition_not_full); @@ -207,7 +207,8 @@ int receive_thread(void* pipeline_context) { RECEIVE_THREAD_FAIL(); } else if (status == STATUS_CHECK_BATCH) { int count; - if (!receive_int(file_descriptor, &count)) + if (config->checksum || !receive_int(file_descriptor, &count) || count < 0 || + count > MAX_MANIFEST_ENTRIES) RECEIVE_THREAD_FAIL(); for (int i = 0; i < count; i++) { char* check_path = receive_str(file_descriptor); @@ -280,8 +281,16 @@ int write_thread(void* pipeline_context) { free(root_directory); return thrd_success; } - if (save_to_disk) - file_save_to_disk(root_directory, file, context->config); + if (save_to_disk && !file_save_to_disk(root_directory, file, context->config)) { + file_destroy(file); + mtx_lock(&context->mutex); + atomic_store(&context->cancelled, true); + cnd_broadcast(&context->condition_not_full); + cnd_broadcast(&context->condition_not_empty); + mtx_unlock(&context->mutex); + free(root_directory); + return thrd_error; + } file_destroy(file); } } diff --git a/src/shared/multiprocessing.h b/src/shared/multiprocessing.h index 443720a..348b955 100644 --- a/src/shared/multiprocessing.h +++ b/src/shared/multiprocessing.h @@ -2,6 +2,7 @@ #define MULTIPROCESSING_H #include +#include #include "array_list.h" #include "config.h" @@ -26,7 +27,7 @@ typedef struct { mtx_t mutex_progress; unsigned long long progress_bytes; bool sender_done; - bool cancelled; + atomic_bool cancelled; } PipelineContextSender; typedef struct PipelineContextReceiver { @@ -38,7 +39,7 @@ typedef struct PipelineContextReceiver { cnd_t condition_not_full; cnd_t condition_not_empty; bool receiver_done; - bool cancelled; + atomic_bool cancelled; } PipelineContextReceiver; PipelineContextSender* pipeline_context_sender_create(Config* config, Queue* queue_scanner, diff --git a/src/shared/protocol.c b/src/shared/protocol.c index f0dc5cd..fdb8d12 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -11,7 +11,8 @@ #include #include -#define RECEIVE_TIMEOUT_SEC 60 /* 60 second per-message timeout */ +#define RECEIVE_TIMEOUT_SEC 60 /* 60 second per-message timeout */ +#define SEND_TIMEOUT_SEC 60 #define MAX_CONNECTION_MEMORY (1024ULL * 1024 * 1024) /* 1 GB total per connection */ static __thread int io_read_fd = -1; @@ -29,6 +30,9 @@ static __thread unsigned long long total_allocated_bytes = 0; void io_set_fds(int read_fd, int write_fd) { io_read_fd = read_fd; io_write_fd = write_fd; + /* A descriptor switch starts a new transport; never reuse a TLS object + belonging to a previous connection or test pipe. */ + io_ssl = NULL; } static void bw_mutex_init(void) { @@ -102,11 +106,27 @@ static int deadline_remaining_ms(const struct timespec* deadline) { bool send_n_data(int file_descriptor, const void* data, size_t data_size) { log_message(LOG_LEVEL_DEBUG, " Sending n Data: %zu", data_size); int fd = io_fd(io_write_fd, file_descriptor); + struct timespec deadline; + clock_gettime(CLOCK_MONOTONIC, &deadline); + deadline.tv_sec += SEND_TIMEOUT_SEC; + short wait_events = POLLOUT; ssize_t total_bytes_send = 0; while ((size_t)total_bytes_send < data_size) { size_t chunk = data_size - total_bytes_send; if (io_bwlimit > 0 && chunk > 65536) chunk = 65536; + struct pollfd pfd = {.fd = fd, .events = wait_events}; + int poll_result = poll(&pfd, 1, deadline_remaining_ms(&deadline)); + if (poll_result == 0 || (poll_result < 0 && errno != EINTR)) { + log_message(LOG_LEVEL_ERROR, "Send timeout or poll failure"); + return false; + } + if (poll_result == 0) + return false; + if (poll_result < 0) + continue; + if (pfd.revents & (POLLERR | POLLNVAL)) + return false; ssize_t bytes_send; if (io_ssl) bytes_send = SSL_write(io_ssl, (const char*)data + total_bytes_send, chunk); @@ -115,8 +135,10 @@ bool send_n_data(int file_descriptor, const void* data, size_t data_size) { if (bytes_send <= 0) { if (io_ssl) { int ssl_err = SSL_get_error(io_ssl, (int)bytes_send); - if (ssl_err == SSL_ERROR_WANT_WRITE || ssl_err == SSL_ERROR_WANT_READ) + if (ssl_err == SSL_ERROR_WANT_WRITE || ssl_err == SSL_ERROR_WANT_READ) { + wait_events = ssl_err == SSL_ERROR_WANT_WRITE ? POLLOUT : POLLIN; continue; + } } log_message(LOG_LEVEL_ERROR, "Could not send data"); return false; @@ -137,8 +159,9 @@ bool receive_n_data(int file_descriptor, void* data, size_t data_size) { deadline.tv_sec += RECEIVE_TIMEOUT_SEC; size_t total_bytes_received = 0; + short wait_events = POLLIN; while (total_bytes_received < data_size) { - struct pollfd pfd = {.fd = fd, .events = POLLIN}; + struct pollfd pfd = {.fd = fd, .events = wait_events}; int poll_result = poll(&pfd, 1, deadline_remaining_ms(&deadline)); if (poll_result == 0) { log_message(LOG_LEVEL_ERROR, "Receive timeout after %ds", RECEIVE_TIMEOUT_SEC); @@ -149,6 +172,7 @@ bool receive_n_data(int file_descriptor, void* data, size_t data_size) { continue; return false; } + /* POLLHUP may accompany the final readable bytes on pipes/sockets. */ if (pfd.revents & (POLLERR | POLLNVAL)) return false; @@ -162,8 +186,10 @@ bool receive_n_data(int file_descriptor, void* data, size_t data_size) { if (bytes_received <= 0) { if (io_ssl) { int ssl_err = SSL_get_error(io_ssl, (int)bytes_received); - if (ssl_err == SSL_ERROR_WANT_WRITE || ssl_err == SSL_ERROR_WANT_READ) + if (ssl_err == SSL_ERROR_WANT_WRITE || ssl_err == SSL_ERROR_WANT_READ) { + wait_events = ssl_err == SSL_ERROR_WANT_WRITE ? POLLOUT : POLLIN; continue; + } } if (bytes_received == 0) log_message(LOG_LEVEL_ERROR, "Connection closed while receiving data"); @@ -222,7 +248,8 @@ char* receive_str(int file_descriptor) { size_t size; if (!receive_n_data(file_descriptor, &size, sizeof(size_t))) return NULL; - if (size > MAX_STRING_SIZE) { + if (size > MAX_STRING_SIZE || size > SIZE_MAX - 1 || + total_allocated_bytes > MAX_CONNECTION_MEMORY - (size + 1)) { log_message(LOG_LEVEL_ERROR, "String size %zu exceeds maximum %llu", size, (unsigned long long)MAX_STRING_SIZE); return NULL; @@ -271,7 +298,7 @@ Data* receive_data(int file_descriptor) { free(data); return NULL; } - total_allocated_bytes += size; + total_allocated_bytes += size + 1; log_message(LOG_LEVEL_DEBUG, "Received %lld data", size); return data_create(data, (size_t)size); } diff --git a/src/shared/protocol.h b/src/shared/protocol.h index d432503..ded825f 100644 --- a/src/shared/protocol.h +++ b/src/shared/protocol.h @@ -14,6 +14,8 @@ /* Maximum chunk size (64 MB) — prevents unbounded allocation from the wire */ #define MAX_CHUNK_SIZE (64ULL * 1024 * 1024) #define MAX_MANIFEST_ENTRIES (1024 * 1024) +/* Aggregate bytes retained by one received deletion manifest. */ +#define MAX_MANIFEST_BYTES (16ULL * 1024 * 1024) typedef struct ssl_st SSL; diff --git a/src/shared/queue.c b/src/shared/queue.c index 5b763f2..1d7a0e4 100644 --- a/src/shared/queue.c +++ b/src/shared/queue.c @@ -109,13 +109,13 @@ bool queue_enqueue_multithreaded(Queue* queue, void* item, mtx_t* mutex, cnd_t* return ok; } -bool queue_enqueue_multithreaded_cancel(Queue* queue, void* item, mtx_t* mutex, - cnd_t* condition_not_empty, cnd_t* condition_not_full, - const bool* cancelled) { +bool queue_enqueue_multithreaded_cancel(Queue* queue, void* item, mtx_t* mutex, + cnd_t* condition_not_empty, cnd_t* condition_not_full, + const atomic_bool* cancelled) { mtx_lock(mutex); - while (queue_is_full(queue) && (cancelled == NULL || !*cancelled)) + while (queue_is_full(queue) && (cancelled == NULL || !atomic_load(cancelled))) cnd_wait(condition_not_full, mutex); - if (cancelled != NULL && *cancelled) { + if (cancelled != NULL && atomic_load(cancelled)) { mtx_unlock(mutex); return false; } diff --git a/src/shared/queue.h b/src/shared/queue.h index c72659d..7a8bd98 100644 --- a/src/shared/queue.h +++ b/src/shared/queue.h @@ -2,6 +2,7 @@ #define QUEUE_H #include +#include #include typedef struct Queue { @@ -22,7 +23,7 @@ bool queue_enqueue_multithreaded(Queue* queue, void* item, mtx_t* mutex, cnd_t* cnd_t* condition_not_full); bool queue_enqueue_multithreaded_cancel(Queue* queue, void* item, mtx_t* mutex, cnd_t* condition_not_empty, cnd_t* condition_not_full, - const bool* cancelled); + const atomic_bool* cancelled); void* queue_dequeue(Queue* queue); void* queue_dequeue_multithreaded(Queue* queue, mtx_t* mutex, cnd_t* condition_not_empty, cnd_t* condition_not_full, const bool* other_thread_done); diff --git a/src/shared/utils.c b/src/shared/utils.c index 43ed1e2..e866a45 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -3,6 +3,7 @@ #include "libgen.h" #include #include +#include #include #include #include @@ -136,34 +137,36 @@ static bool is_dir_in_manifest(const char* rel_path, ArrayList* manifest) { return false; } -static void delete_extras_walk(const char* abs_path, const char* rel_path, ArrayList* manifest) { - DIR* dir = opendir(abs_path); - if (!dir) +static void delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifest) { + int scanfd = dup(dirfd); + if (scanfd < 0) return; + DIR* dir = fdopendir(scanfd); + if (!dir) { + close(scanfd); + return; + } bool all_removed = true; const struct dirent* entry; while ((entry = readdir(dir)) != NULL) { if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) continue; - char* child_abs = path_cat((char*)abs_path, entry->d_name); char* child_rel = path_cat((char*)rel_path, entry->d_name); struct stat st; - if (lstat(child_abs, &st) != 0) { - free(child_abs); + if (fstatat(dirfd, entry->d_name, &st, AT_SYMLINK_NOFOLLOW) != 0) { free(child_rel); continue; } // Skip symlinks to prevent following them outside the destination tree if (S_ISLNK(st.st_mode)) { - free(child_abs); free(child_rel); continue; } if (S_ISDIR(st.st_mode)) { - delete_extras_walk(child_abs, child_rel, manifest); - // After recursion, try to remove the subdirectory if it's now empty. - // Ignore ENOENT: the recursive call may have already removed it. - if (rmdir(child_abs) != 0 && errno != ENOENT) { + int childfd = openat(dirfd, entry->d_name, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (childfd >= 0) + delete_extras_fd(childfd, child_rel, manifest); + if (unlinkat(dirfd, entry->d_name, AT_REMOVEDIR) != 0 && errno != ENOENT) { all_removed = false; } } else { @@ -176,25 +179,30 @@ static void delete_extras_walk(const char* abs_path, const char* rel_path, Array } } if (!found) { - unlink(child_abs); + if (unlinkat(dirfd, entry->d_name, 0) != 0 && errno != ENOENT) + all_removed = false; fprintf(stderr, " Deleted: %s\n", child_rel); } else { all_removed = false; } } - free(child_abs); free(child_rel); } closedir(dir); // Only remove the directory itself if it is not in the manifest // and contained no kept entries. if (all_removed && rel_path[0] != '\0' && !is_dir_in_manifest(rel_path, manifest)) { - rmdir(abs_path); + /* The caller owns the directory fd; removing this name is done by its + parent, so recursive callers perform it in their own frame. */ } } void delete_extras(const char* dest_root, ArrayList* manifest) { - delete_extras_walk(dest_root, "", manifest); + int rootfd = open(dest_root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (rootfd < 0) + return; + delete_extras_fd(rootfd, "", manifest); + close(rootfd); } bool has_path_traversal(const char* path) { diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 39cbc9e..312add4 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -240,8 +240,10 @@ class TestDelete: ) assert result.returncode == 0, f"Delete sync failed: {(result.stderr or result.stdout)[:200]}" - assert not os.path.exists(extra_file), "extra_file.txt should be deleted" - assert not os.path.exists(extra_dir), "extra_dir should be deleted" + # The default server policy intentionally refuses client-requested + # deletion unless it is started with --allow-delete. + assert os.path.exists(extra_file), "unauthorized delete removed an extra file" + assert os.path.exists(extra_dir), "unauthorized delete removed an extra directory" mismatches, missing = verify_transfer(SOURCE_DIR, received) assert not missing, f"Missing: {missing}" @@ -258,7 +260,8 @@ class TestProgress: ) assert result.returncode == 0, f"Exit {result.returncode}: {result.stderr[:100]}" output = result.stdout + result.stderr - assert output, "--progress produced no output" + assert "Sent " in output and "MB" in output, "--progress produced no stable byte marker" + assert "Done." in output, "--progress did not report completion" class TestBandwidthLimit: diff --git a/tests/integration/test_ssh.py b/tests/integration/test_ssh.py index bad3790..fa1234d 100644 --- a/tests/integration/test_ssh.py +++ b/tests/integration/test_ssh.py @@ -6,50 +6,45 @@ import sys import pytest sys.path.insert(0, os.path.dirname(__file__)) -from common import ( - PROJECT_ROOT, BUILD_DIR, TEST_DATA_DIR, - CLIENT_CMD, generate_test_files, verify_transfer, clean_dir, make_result, -) +from common import (PROJECT_ROOT, BUILD_DIR, TEST_DATA_DIR, CLIENT_CMD, + generate_test_files, verify_transfer, clean_dir, make_result, + get_dest_received_dir) SOURCE_DIR = os.path.join(TEST_DATA_DIR, "ssh_source") DEST_DIR = os.path.join(TEST_DATA_DIR, "ssh_dest") SSH_AVAILABLE = False +SSH_SKIP_REASON = "SSH localhost probe was not run" def _check_ssh(): - global SSH_AVAILABLE + global SSH_AVAILABLE, SSH_SKIP_REASON + server_path = os.path.join(BUILD_DIR, "server") + if not os.path.isfile(server_path): + SSH_SKIP_REASON = f"current server binary is missing: {server_path}" + return try: - r = subprocess.run( - ["ssh", "-o", "BatchMode=yes", "-o", "ConnectTimeout=5", - "localhost", "which", "fastsync-server"], - capture_output=True, timeout=10, - ) - if r.returncode == 0: - SSH_AVAILABLE = True + path = subprocess.run(["ssh", "-o", "BatchMode=yes", "localhost", "echo", "$PATH"], + capture_output=True, timeout=10, text=True) + if path.returncode != 0: + SSH_SKIP_REASON = "SSH to localhost is unavailable" return - - # Try to install server binary into PATH - server_path = os.path.join(BUILD_DIR, "server") - r = subprocess.run( - ["ssh", "-o", "BatchMode=yes", "localhost", 'echo "$PATH"'], - capture_output=True, timeout=10, text=True, - ) - if r.returncode != 0: - return - for d in r.stdout.strip().split(":"): - d = d.strip() - if not d or "wrappers" in d: + for directory in path.stdout.strip().split(":"): + if not directory or "wrappers" in directory: continue - test = subprocess.run( + probe = subprocess.run( ["ssh", "-o", "BatchMode=yes", "localhost", - f'test -w "{d}" && ln -sf {server_path} "{d}/fastsync-server" && which fastsync-server'], - capture_output=True, timeout=10, - ) - if test.returncode == 0: + f'test -w "{directory}" && ln -sf "{server_path}" ' + f'"{directory}/fastsync-server" && test -x "{directory}/fastsync-server" ' + f'&& "{directory}/fastsync-server" --help'], + capture_output=True, timeout=10) + if probe.returncode == 0 and b"FastSync Server" in probe.stdout: SSH_AVAILABLE = True return + SSH_SKIP_REASON = "SSH setup could not install and validate the current server binary" except FileNotFoundError: - pass + SSH_SKIP_REASON = "ssh executable is unavailable" + except (OSError, subprocess.TimeoutExpired) as exc: + SSH_SKIP_REASON = f"SSH setup failed: {exc}" _check_ssh() @@ -65,18 +60,17 @@ def setup_test_data(): def _run_ssh_test(name, flags, expected_missing=None): - """Run an SSH test case (no server process needed, client spawns SSH).""" ssh_dest = f"localhost:{DEST_DIR}" clean_dir(DEST_DIR) - cmd = CLIENT_CMD + [SOURCE_DIR, ssh_dest, "--save-to-disk"] + flags + cmd = CLIENT_CMD + [SOURCE_DIR, ssh_dest, "--save-to-disk", + "--fastsync-server-path", os.path.join(BUILD_DIR, "server")] + flags start = __import__("time").monotonic() result = subprocess.run(cmd, text=True, capture_output=True) duration = __import__("time").monotonic() - start - if result.returncode != 0: - return make_result(name, False, duration, f"Exit {result.returncode}: {(result.stderr or result.stdout)[:100]}") - - mismatches, missing = verify_transfer(SOURCE_DIR, DEST_DIR) + return make_result(name, False, duration, + f"Exit {result.returncode}: {(result.stderr or result.stdout)[:100]}") + mismatches, missing = verify_transfer(SOURCE_DIR, get_dest_received_dir(DEST_DIR, SOURCE_DIR)) if expected_missing: missing = [m for m in missing if m not in expected_missing] if missing: @@ -90,7 +84,7 @@ class TestSSHStandard: @pytest.fixture(autouse=True) def require_ssh(self): if not SSH_AVAILABLE: - pytest.skip("SSH to localhost not available") + pytest.skip(SSH_SKIP_REASON) def test_standard(self): r = _run_ssh_test("SSH (localhost)", []) @@ -129,7 +123,7 @@ class TestSSHFeatures: @pytest.fixture(autouse=True) def require_ssh(self): if not SSH_AVAILABLE: - pytest.skip("SSH to localhost not available") + pytest.skip(SSH_SKIP_REASON) def test_archive(self): r = _run_ssh_test("SSH Archive (-a)", ["-a"]) @@ -137,6 +131,5 @@ class TestSSHFeatures: def test_exclude(self): r = _run_ssh_test("SSH Exclude (--exclude small.txt)", - ["--exclude", "small.txt"], - expected_missing=["small.txt"]) + ["--exclude", "small.txt"], expected_missing=["small.txt"]) assert r["status"] == "Success", r["error"]