From f3dd2e326eb6add187298099be03161dc8756392 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 20 Jul 2026 18:44:10 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20memory/null=20safety=20bugs=20=E2=80=94?= =?UTF-8?q?=20issues=20#74,=20#72,=20#69,=20#64,=20#60,=20#50,=20#49,=20#6?= =?UTF-8?q?5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- _deps/xxhash-src | 1 + 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 ++++++++++++ 10 files changed, 145 insertions(+), 18 deletions(-) create mode 160000 _deps/xxhash-src diff --git a/_deps/xxhash-src b/_deps/xxhash-src new file mode 160000 index 0000000..e626a72 --- /dev/null +++ b/_deps/xxhash-src @@ -0,0 +1 @@ +Subproject commit e626a72bc2321cd320e953a0ccf1584cad60f363 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..f498492 100644 --- a/src/shared/protocol.h +++ b/src/shared/protocol.h @@ -36,3 +36,6 @@ bool send_status(int file_descriptor, Status status); bool receive_status(int file_descriptor, Status* status); #endif + +/* Maximum allowed string size for receive_str (10 MB) */ +#define MAX_STRING_SIZE (10 * 1024 * 1024) 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(); }