From 990c4362af951fa82051e8a81b268f0d51859094 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 18:44:10 +0200 Subject: [PATCH 01/11] =?UTF-8?q?fix:=20memory/null=20safety=20bugs=20?= =?UTF-8?q?=E2=80=94=20issues=20#74,=20#72,=20#69,=20#64,=20#60,=20#50,=20?= =?UTF-8?q?#49,=20#65?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/client/scanner.c | 59 ++++++++++++++++++++++++++++++++++++-- src/shared/compression.c | 15 ++++++++-- src/shared/data.c | 4 ++- src/shared/protocol.c | 9 ++++++ src/shared/protocol.h | 3 ++ src/shared/transport_ssh.c | 14 ++++++++- src/shared/utils.c | 31 +++++++++++++------- tests/test_data.c | 9 ++++++ tests/test_protocol.c | 18 ++++++++++++ 9 files changed, 144 insertions(+), 18 deletions(-) diff --git a/src/client/scanner.c b/src/client/scanner.c index ad56fb2..109ffa0 100644 --- a/src/client/scanner.c +++ b/src/client/scanner.c @@ -24,9 +24,58 @@ DirectoryScanner* directory_scanner_create(char* root_directory, bool use_metada scanner->current_path = NULL; scanner->use_metadata = use_metadata; scanner->chunk_size = chunk_size > 0 ? chunk_size : DESIRED_CHUNK_SIZE; - scanner->exclude_patterns = exclude_patterns; + /* Deep-copy exclude patterns */ + if (exclude_count > 0 && exclude_patterns != NULL) { + scanner->exclude_patterns = malloc((size_t)exclude_count * sizeof(char*)); + if (scanner->exclude_patterns == NULL) { + queue_destroy(scanner->directories); + free(scanner); + return NULL; + } + for (int i = 0; i < exclude_count; i++) { + scanner->exclude_patterns[i] = str_dup(exclude_patterns[i]); + if (scanner->exclude_patterns[i] == NULL) { + for (int j = 0; j < i; j++) + free(scanner->exclude_patterns[j]); + free(scanner->exclude_patterns); + queue_destroy(scanner->directories); + free(scanner); + return NULL; + } + } + } else { + scanner->exclude_patterns = NULL; + } scanner->exclude_count = exclude_count; - scanner->include_patterns = include_patterns; + + /* Deep-copy include patterns */ + if (include_count > 0 && include_patterns != NULL) { + scanner->include_patterns = malloc((size_t)include_count * sizeof(char*)); + if (scanner->include_patterns == NULL) { + for (int i = 0; i < exclude_count; i++) + free(scanner->exclude_patterns[i]); + free(scanner->exclude_patterns); + queue_destroy(scanner->directories); + free(scanner); + return NULL; + } + for (int i = 0; i < include_count; i++) { + scanner->include_patterns[i] = str_dup(include_patterns[i]); + if (scanner->include_patterns[i] == NULL) { + for (int j = 0; j < i; j++) + free(scanner->include_patterns[j]); + free(scanner->include_patterns); + for (int j = 0; j < exclude_count; j++) + free(scanner->exclude_patterns[j]); + free(scanner->exclude_patterns); + queue_destroy(scanner->directories); + free(scanner); + return NULL; + } + } + } else { + scanner->include_patterns = NULL; + } scanner->include_count = include_count; scanner->max_size = max_size; scanner->min_size = min_size; @@ -42,6 +91,12 @@ void directory_scanner_destroy(DirectoryScanner* scanner) { scanner->current_dir = NULL; } free(scanner->current_path); + for (int i = 0; i < scanner->exclude_count; i++) + free(scanner->exclude_patterns[i]); + free(scanner->exclude_patterns); + for (int i = 0; i < scanner->include_count; i++) + free(scanner->include_patterns[i]); + free(scanner->include_patterns); queue_destroy(scanner->directories); free(scanner); } diff --git a/src/shared/compression.c b/src/shared/compression.c index 3641859..01b558f 100644 --- a/src/shared/compression.c +++ b/src/shared/compression.c @@ -1,7 +1,8 @@ #include "compression.h" #include "data.h" #include "log.h" -#include "stdlib.h" +#include +#include #include "zstd.h" #define INITIAL_DECOMPRESS_BUF_SIZE (1024 * 1024) @@ -66,8 +67,16 @@ Data* data_decompress(Data* compressed_data) { return NULL; } - size_t buf_size = - (!ZSTD_isError(dst_size) && dst_size > 0) ? (size_t)dst_size : INITIAL_DECOMPRESS_BUF_SIZE; + size_t buf_size = INITIAL_DECOMPRESS_BUF_SIZE; + if (!ZSTD_isError(dst_size) && dst_size > 0) { + if (dst_size > SIZE_MAX) { + log_message(LOG_LEVEL_ERROR, + "Decompressed size %llu exceeds addressable memory, using fallback buffer", + dst_size); + } else { + buf_size = (size_t)dst_size; + } + } Data* uncompressed_data = data_create_empty(buf_size); if (!uncompressed_data) { log_message(LOG_LEVEL_ERROR, "Failed to allocate decompression buffer"); diff --git a/src/shared/data.c b/src/shared/data.c index 5b3dcba..e0e3156 100644 --- a/src/shared/data.c +++ b/src/shared/data.c @@ -3,7 +3,9 @@ #include "stdlib.h" Data* data_create_empty(size_t data_size) { - void* data = malloc(data_size); + /* malloc(0) is UB; allocate at least 1 byte but preserve requested size */ + size_t alloc_size = data_size > 0 ? data_size : 1; + void* data = malloc(alloc_size); if (data == NULL) { log_message(LOG_LEVEL_ERROR, "Could not allocate memory for empty data"); return NULL; diff --git a/src/shared/protocol.c b/src/shared/protocol.c index 4d91ce6..c3dcb74 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -138,6 +138,10 @@ static const char* status_to_string(Status status) { } bool send_str(int file_descriptor, const char* data) { + if (data == NULL) { + log_message(LOG_LEVEL_ERROR, "send_str called with NULL data"); + return false; + } size_t size = strlen(data); if (!send_n_data(file_descriptor, &size, sizeof(size_t))) return false; @@ -151,6 +155,11 @@ 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) { + log_message(LOG_LEVEL_ERROR, "receive_str: size %zu exceeds maximum %zu", size, + (size_t)MAX_STRING_SIZE); + return NULL; + } char* data = (char*)malloc(size + 1); if (data == NULL) return NULL; diff --git a/src/shared/protocol.h b/src/shared/protocol.h index a7854f1..bc57d6b 100644 --- a/src/shared/protocol.h +++ b/src/shared/protocol.h @@ -5,6 +5,9 @@ #include #include +/* Maximum allowed string size for receive_str (10 MB) */ +#define MAX_STRING_SIZE (10 * 1024 * 1024) + typedef struct ssl_st SSL; typedef int Status; diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index 0462ce4..4ff2bb8 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -124,7 +124,10 @@ Client* client_connect_ssh(const char* destination, int port) { else snprintf(ssh_user, sizeof(ssh_user), "%s", r.host); - char* ssh_argv[16]; + size_t ssh_argv_max = 32; + char** ssh_argv = calloc(ssh_argv_max, sizeof(char*)); + if (ssh_argv == NULL) + _exit(1); int ac = 0; char port_str[16]; ssh_argv[ac++] = "ssh"; @@ -135,15 +138,24 @@ Client* client_connect_ssh(const char* destination, int port) { ssh_argv[ac++] = "-o"; ssh_argv[ac++] = "ControlPath=~/.cache/fastsync-%r@%h:%p"; if (port > 0 && port != 22) { + if ((size_t)ac + 2 >= ssh_argv_max) { + free(ssh_argv); + _exit(1); + } ssh_argv[ac++] = "-p"; snprintf(port_str, sizeof(port_str), "%d", port); ssh_argv[ac++] = port_str; } + if ((size_t)ac + 3 >= ssh_argv_max) { + free(ssh_argv); + _exit(1); + } ssh_argv[ac++] = ssh_user; ssh_argv[ac++] = "fastsync-server"; ssh_argv[ac++] = "--stdio"; ssh_argv[ac] = NULL; execvp("ssh", ssh_argv); + free(ssh_argv); perror("exec of ssh failed"); ssize_t wret = write(exec_pipe[1], "x", 1); (void)wret; diff --git a/src/shared/utils.c b/src/shared/utils.c index ad1e533..528191b 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -10,19 +10,23 @@ #include bool mkdir_r(const char* path) { - char* path_duplicate = malloc(strlen(path) + 1); + size_t path_len = strlen(path); + char* path_duplicate = malloc(path_len + 1); if (!path_duplicate) return false; - strcpy(path_duplicate, path); - char* path_current = (char*)malloc((strlen(path) + 2) * sizeof(char)); + memcpy(path_duplicate, path, path_len + 1); + /* Buffer for building subpaths: path_len + 1 for leading '/' + 1 for null */ + size_t buf_size = path_len + 2; + char* path_current = (char*)malloc(buf_size); if (!path_current) { free(path_duplicate); return false; } - char* path_current_position = path_current; + size_t pos = 0; if (path[0] == '/') { - strcpy(path_current, "/"); - path_current_position += 1; + path_current[0] = '/'; + path_current[1] = '\0'; + pos = 1; } else { path_current[0] = '\0'; } @@ -31,10 +35,16 @@ bool mkdir_r(const char* path) { const char* part = strtok_r(path_duplicate, delimiter, &saveptr); bool ok = true; while (part != NULL) { - strcpy(path_current_position, part); - path_current_position += strlen(part) * sizeof(char); - strcpy(path_current_position, "/"); - path_current_position += sizeof(char); + size_t part_len = strlen(part); + if (pos + part_len + 1 >= buf_size) { + ok = false; + break; + } + memcpy(path_current + pos, part, part_len); + pos += part_len; + path_current[pos] = '/'; + pos++; + path_current[pos] = '\0'; struct stat st; if (stat(path_current, &st) != 0) { if (mkdir(path_current, 0755) != 0) { @@ -49,7 +59,6 @@ bool mkdir_r(const char* path) { free(path_current); return ok; } - char* str_dup(const char* string) { if (string == NULL) return NULL; diff --git a/tests/test_data.c b/tests/test_data.c index 33c4888..266a9ad 100644 --- a/tests/test_data.c +++ b/tests/test_data.c @@ -23,6 +23,14 @@ static void test_data_create_empty() { data_destroy(d); } +static void test_data_create_empty_zero() { + Data* d = data_create_empty(0); + EXPECT_NOT_NULL(d); + EXPECT_NOT_NULL(d->data); + EXPECT_EQ_INT((int)d->size, 0); + data_destroy(d); +} + static void test_data_create_reserve() { Data* d = data_create_reserve(1024); EXPECT_NOT_NULL(d); @@ -44,6 +52,7 @@ static void test_data_destroy_normal() { void test_data() { test_data_create(); test_data_create_empty(); + test_data_create_empty_zero(); test_data_create_reserve(); test_data_destroy_null(); test_data_destroy_normal(); diff --git a/tests/test_protocol.c b/tests/test_protocol.c index d0070fe..09c2a46 100644 --- a/tests/test_protocol.c +++ b/tests/test_protocol.c @@ -169,6 +169,23 @@ static void test_receive_str_truncated() { close(p[0]); } +static void test_receive_str_oversized() { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + /* Send a size exceeding MAX_STRING_SIZE */ + size_t huge = MAX_STRING_SIZE + 1; + EXPECT_TRUE(send_n_data(0, &huge, sizeof(size_t))); + + char* received = receive_str(0); + EXPECT_NULL(received); + + close(p[0]); + close(p[1]); +} + void test_protocol() { test_send_receive_n_data(); test_send_receive_n_data_zero(); @@ -179,4 +196,5 @@ void test_protocol() { test_send_receive_status(); test_receive_n_data_truncated(); test_receive_str_truncated(); + test_receive_str_oversized(); } From 788d3c7bead36197ba054e332326e02aa2ee6760 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 19:48:31 +0200 Subject: [PATCH 02/11] ci: trigger CI on PR From ba0afd41521bd0f24064a55c5cd00675164c3b9e Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 19:53:42 +0200 Subject: [PATCH 03/11] fix: cppcheck and clang-format fixes --- src/shared/transport_ssh.c | 9 ++------- tests/test_protocol.c | 2 +- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index 4ff2bb8..cefcfe1 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -138,24 +138,19 @@ Client* client_connect_ssh(const char* destination, int port) { ssh_argv[ac++] = "-o"; ssh_argv[ac++] = "ControlPath=~/.cache/fastsync-%r@%h:%p"; if (port > 0 && port != 22) { - if ((size_t)ac + 2 >= ssh_argv_max) { - free(ssh_argv); + if ((size_t)ac + 2 >= ssh_argv_max) _exit(1); - } ssh_argv[ac++] = "-p"; snprintf(port_str, sizeof(port_str), "%d", port); ssh_argv[ac++] = port_str; } - if ((size_t)ac + 3 >= ssh_argv_max) { - free(ssh_argv); + if ((size_t)ac + 3 >= ssh_argv_max) _exit(1); - } ssh_argv[ac++] = ssh_user; ssh_argv[ac++] = "fastsync-server"; ssh_argv[ac++] = "--stdio"; ssh_argv[ac] = NULL; execvp("ssh", ssh_argv); - free(ssh_argv); perror("exec of ssh failed"); ssize_t wret = write(exec_pipe[1], "x", 1); (void)wret; diff --git a/tests/test_protocol.c b/tests/test_protocol.c index 09c2a46..bece803 100644 --- a/tests/test_protocol.c +++ b/tests/test_protocol.c @@ -179,7 +179,7 @@ static void test_receive_str_oversized() { size_t huge = MAX_STRING_SIZE + 1; EXPECT_TRUE(send_n_data(0, &huge, sizeof(size_t))); - char* received = receive_str(0); + const char* received = receive_str(0); EXPECT_NULL(received); close(p[0]); From 5e8d0a2dbf31ca986960ae08ceb40dbf6cb4e8b0 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 20:11:23 +0200 Subject: [PATCH 04/11] ci: re-trigger after review fixes From e3bd7a8cdf1e351baaa79b647ab153d6b032c36a Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 20:28:16 +0200 Subject: [PATCH 05/11] ci: re-trigger after fixes From 9042dfcfa945275a6da2c04b650526bb5db28b3c Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 20:41:16 +0200 Subject: [PATCH 06/11] ci: re-trigger after cppcheck fixes From 262a4362647e028cd83f4d898374ff1f3af1e99c Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 20:48:22 +0200 Subject: [PATCH 07/11] fix: auto-detect valgrind to skip fork tests --- tests/test_file.c | 2 +- tests/test_utils.h | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_file.c b/tests/test_file.c index 1a65a05..ae50a30 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -271,7 +271,7 @@ void test_file() { test_to_disk_basic(); test_to_disk_creates_dirs(); test_file_content_to_buffer(); - if (!getenv("FASTSYNC_UNDER_VALGRIND")) { + if (!is_running_under_valgrind()) { // Fork tests are skipped under valgrind because the parent process runs // orders of magnitude slower than the child (parent is instrumented, child // is not), which causes pipe-based protocol handshake timeouts. The parent diff --git a/tests/test_utils.h b/tests/test_utils.h index c9e9f02..67b6bca 100644 --- a/tests/test_utils.h +++ b/tests/test_utils.h @@ -2,9 +2,24 @@ #define TEST_UTILS_H #include +#include #include #include +// Detect if running under valgrind by checking /proc/self/maps for vgpreload. +// This is used to skip fork-based tests that are incompatible with valgrind +// (the instrumented parent runs too slowly, causing pipe timeouts). +static inline bool is_running_under_valgrind(void) { + FILE* f = fopen("/proc/self/maps", "r"); + if (!f) + return false; + char buf[4096]; + size_t n = fread(buf, 1, sizeof(buf) - 1, f); + fclose(f); + buf[n] = '\0'; + return strstr(buf, "vgpreload") != NULL; +} + // Global test suite status extern int tests_run; extern int tests_failed; From 83c61aadc2eff97ad4230e7e12432a9db3498f06 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 22:04:45 +0200 Subject: [PATCH 08/11] ci: trigger CI From 23af379bfb8c151a84657741c8c4c8a893e13f13 Mon Sep 17 00:00:00 2001 From: TapTap Date: Tue, 21 Jul 2026 14:16:09 +0200 Subject: [PATCH 09/11] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20unused=20var,=20constness,=20format=20specifiers,?= =?UTF-8?q?=20bounds=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/client/scanner.c | 2 +- src/shared/data.c | 2 +- src/shared/protocol.c | 4 ++-- src/shared/utils.c | 7 +++---- src/shared/utils.h | 2 +- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/client/scanner.c b/src/client/scanner.c index 109ffa0..c4708a5 100644 --- a/src/client/scanner.c +++ b/src/client/scanner.c @@ -159,7 +159,7 @@ Chunk* directory_scanner_next(DirectoryScanner* scanner) { char* cur_path = path_cat(scanner->current_path, entry->d_name); struct stat stats; - if (stat(cur_path, &stats) != 0) { + if (lstat(cur_path, &stats) != 0) { free(cur_path); continue; } diff --git a/src/shared/data.c b/src/shared/data.c index e0e3156..82e6198 100644 --- a/src/shared/data.c +++ b/src/shared/data.c @@ -1,6 +1,6 @@ #include "data.h" #include "log.h" -#include "stdlib.h" +#include Data* data_create_empty(size_t data_size) { /* malloc(0) is UB; allocate at least 1 byte but preserve requested size */ diff --git a/src/shared/protocol.c b/src/shared/protocol.c index c3dcb74..4146078 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -178,7 +178,7 @@ bool send_data(int file_descriptor, const Data* data) { return false; if (!send_n_data(file_descriptor, data->data, data_size)) return false; - log_message(LOG_LEVEL_DEBUG, "Send %lld data", data_size); + log_message(LOG_LEVEL_DEBUG, "Send %llu data", data_size); return true; } @@ -193,7 +193,7 @@ Data* receive_data(int file_descriptor) { free(data); return NULL; } - log_message(LOG_LEVEL_DEBUG, "Received %lld data", size); + log_message(LOG_LEVEL_DEBUG, "Received %llu data", size); return data_create(data, (size_t)size); } diff --git a/src/shared/utils.c b/src/shared/utils.c index 528191b..4848052 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -158,18 +158,17 @@ void delete_extras(const char* dest_root, ArrayList* manifest) { delete_extras_walk(dest_root, "", manifest); } -char* path_cat(const char* path1, char* path2) { +char* path_cat(const char* path1, const char* path2) { if (path1 == NULL || *path1 == '\0') return str_dup(path2); if (path2 == NULL || *path2 == '\0') return str_dup(path1); int path1_len = strlen(path1); int path2_len = strlen(path2); - char* path2_pointer = path2; if (path1[path1_len - 1] == '/') path1_len -= 1; if (path2[0] == '/') { - path2_pointer += 1; + path2++; path2_len -= 1; } char* new_path = malloc(path1_len + path2_len + 2); @@ -177,7 +176,7 @@ char* path_cat(const char* path1, char* path2) { return NULL; memcpy(new_path, path1, path1_len); new_path[path1_len] = '/'; - memcpy(new_path + path1_len + 1, path2_pointer, path2_len); + memcpy(new_path + path1_len + 1, path2, path2_len); new_path[path1_len + path2_len + 1] = '\0'; return new_path; } diff --git a/src/shared/utils.h b/src/shared/utils.h index 13cd999..1cc8a6c 100644 --- a/src/shared/utils.h +++ b/src/shared/utils.h @@ -6,7 +6,7 @@ bool mkdir_r(const char* path); char* str_dup(const char* string); -char* path_cat(const char* path1, char* path2); +char* path_cat(const char* path1, const char* path2); bool glob_match(const char* pattern, const char* str); void delete_extras(const char* dest_root, ArrayList* manifest); From ddfd0f1cc2b97822887d92c9a62d7c93df5c1d84 Mon Sep 17 00:00:00 2001 From: TapTap Date: Tue, 21 Jul 2026 14:37:05 +0200 Subject: [PATCH 10/11] fix: add MAX_DATA_SIZE bounds check in receive_data --- src/shared/protocol.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/shared/protocol.c b/src/shared/protocol.c index 4146078..1e5cb7f 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -182,10 +182,16 @@ bool send_data(int file_descriptor, const Data* data) { return true; } +#define MAX_DATA_SIZE (1024ULL * 1024 * 1024) + Data* receive_data(int file_descriptor) { unsigned long long size = 0; if (!receive_n_data(file_descriptor, &size, sizeof(unsigned long long))) return NULL; + if ((size_t)size != size || size > MAX_DATA_SIZE) { + log_message(LOG_LEVEL_ERROR, "receive_data size %llu exceeds limits", size); + return NULL; + } void* data = malloc((size_t)size); if (data == NULL) return NULL; From 94ff55f25647f8ff98b535a3c9caf77d069899eb Mon Sep 17 00:00:00 2001 From: TapTap Date: Tue, 21 Jul 2026 14:39:35 +0200 Subject: [PATCH 11/11] fix: const qualifier for dirent pointer (cppcheck) --- src/shared/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/utils.c b/src/shared/utils.c index 4848052..9f9ad5b 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -108,7 +108,7 @@ static void delete_extras_walk(const char* abs_path, const char* rel_path, Array if (!dir) return; bool all_removed = true; - struct dirent* entry; + const struct dirent* entry; while ((entry = readdir(dir)) != NULL) { if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) continue;