From f2c89b6e7c08dc261204ae3537bd399d786385ea Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 21 Sep 2026 18:52:46 +0200 Subject: [PATCH] fix: mutex leak, errno-after-free, log_perror misuse, status validation - multiprocessing: destroy mutex_progress on the dir_entries_mutex init-failure path (init >= 7); drop bogus log_perror - delete_plan: capture errno before free() in apply_deferred_path - queue/array_list: log_message instead of log_perror for non-errno conditions - protocol: reject unknown wire Status values via status_is_valid() in receive_status, receive_status_timed and the keepalive reader; declare protocol_receive_status_timed in protocol.h - protocol: %llu for unsigned long long debug counters - tests: out-of-range status rejection test --- src/shared/array_list.c | 6 +++--- src/shared/delete_plan.c | 3 ++- src/shared/multiprocessing.c | 4 +++- src/shared/protocol.c | 25 +++++++++++++++++++++++-- src/shared/protocol.h | 3 +++ src/shared/queue.c | 4 ++-- tests/test_protocol.c | 33 +++++++++++++++++++++++++++++++++ 7 files changed, 69 insertions(+), 9 deletions(-) diff --git a/src/shared/array_list.c b/src/shared/array_list.c index 7813e6d..8606538 100644 --- a/src/shared/array_list.c +++ b/src/shared/array_list.c @@ -9,7 +9,7 @@ ArrayList* array_list_create(void (*item_destroyer)(void* item)) { ArrayList* list = (ArrayList*)protocol_alloc(sizeof(ArrayList)); if (list == NULL) { - log_perror("ERROR: Could not allocate memory for array list struct"); + log_message(LOG_LEVEL_ERROR, "%s", "ERROR: Could not allocate memory for array list struct"); return NULL; } @@ -47,7 +47,7 @@ static bool array_list_extend(ArrayList* array_list) { new_capacity = INITIAL_ARRAY_SIZE; void* new_items = protocol_realloc(array_list->items, new_capacity * sizeof(void*)); if (new_items == NULL) { - log_perror("ERROR: Could not reallocate memory for array list items"); + log_message(LOG_LEVEL_ERROR, "%s", "ERROR: Could not reallocate memory for array list items"); return false; } array_list->items = new_items; @@ -73,7 +73,7 @@ void** array_list_to_array(const ArrayList* array_list) { } void** array = protocol_alloc(array_list->size * sizeof(void*)); if (array == NULL) { - log_perror("Could not malloc space for array from array list!"); + log_message(LOG_LEVEL_ERROR, "%s", "Could not malloc space for array from array list!"); return NULL; } memcpy(array, array_list->items, array_list->size * sizeof(void*)); diff --git a/src/shared/delete_plan.c b/src/shared/delete_plan.c index ae59bc9..58eb52e 100644 --- a/src/shared/delete_plan.c +++ b/src/shared/delete_plan.c @@ -988,10 +988,11 @@ static bool apply_deferred_path(DeletePlanSession* session, const Config* config return false; char* leaf = NULL; int parent_fd = file_open_secure_parent(full, &leaf, false); + int open_errno = errno; free(full); if (parent_fd < 0) { free(leaf); - return errno == ENOENT || errno == ENOTDIR; + return open_errno == ENOENT || open_errno == ENOTDIR; } struct stat st; if (fstatat(parent_fd, leaf, &st, AT_SYMLINK_NOFOLLOW) != 0) { diff --git a/src/shared/multiprocessing.c b/src/shared/multiprocessing.c index 307c736..60f87ee 100644 --- a/src/shared/multiprocessing.c +++ b/src/shared/multiprocessing.c @@ -86,11 +86,13 @@ PipelineContextSender* pipeline_context_sender_create(Config* config, Queue* que return context; fail: - log_perror("Error initializing synchronization objects"); + log_message(LOG_LEVEL_ERROR, "%s", "Error initializing synchronization objects"); if (context->dir_entries_mutex_init) mtx_destroy(&context->dir_entries_mutex); if (context->dir_entries) array_list_delete(context->dir_entries); + if (init >= 7) + mtx_destroy(&context->mutex_progress); if (init >= 6) cnd_destroy(&context->condition_not_empty_loader); if (init >= 5) diff --git a/src/shared/protocol.c b/src/shared/protocol.c index a78fa0a..581ebb8 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -564,6 +564,15 @@ static const char* status_to_string(Status status) { } } +/* Reject a raw wire status outside the known enum range before it is handed to + * callers, so an unknown/corrupt frame fails as a protocol error instead of + * being silently interpreted as an unexpected-but-valid verdict. STATUS_OK is + * the first enumerator and STATUS_STATS the last, so the range check accepts + * every status the protocol defines. */ +static bool status_is_valid(Status status) { + return status >= STATUS_OK && status <= STATUS_STATS; +} + /* Shared string send/receive implementation. `redact` selects whether the * payload body is written to the LOG_DEBUG_PROTO debug log: daemon auth material * (the username and the proof/signature fields) sets it so a --verbose log never @@ -647,7 +656,7 @@ bool protocol_send_data(ProtocolSession* session, const Data* data) { return false; if (!protocol_send_n_data(session, data->data, data_size)) return false; - log_debug_message(LOG_DEBUG_PROTO, "Send %lld data", data_size); + log_debug_message(LOG_DEBUG_PROTO, "Send %llu data", data_size); return true; } @@ -681,7 +690,7 @@ Data* protocol_receive_data_limited(ProtocolSession* session, unsigned long long protocol_release_memory_for_session(session, allocation_size); return NULL; } - log_debug_message(LOG_DEBUG_PROTO, "Received %lld data", size); + log_debug_message(LOG_DEBUG_PROTO, "Received %llu data", size); Data* result = data_create(data, (size_t)size); if (!result) { protocol_release_memory_for_session(session, allocation_size); @@ -791,6 +800,10 @@ bool protocol_receive_status(ProtocolSession* session, Status* status) { } if (!protocol_receive_n_data_until(session, status, sizeof(Status), deadline_ptr)) return false; + if (!status_is_valid(*status)) { + log_message(LOG_LEVEL_ERROR, "Received unknown protocol status %d", *status); + return false; + } if (!protocol_capture_error_detail(session, status, deadline_ptr, NULL)) return false; log_debug_message(LOG_DEBUG_PROTO, "Received Status: %s", status_to_string(*status)); @@ -812,6 +825,10 @@ bool protocol_receive_status_timed(ProtocolSession* session, Status* status, int deadline.tv_sec += timeout_sec; if (!protocol_receive_n_data_until(session, status, sizeof(Status), &deadline)) return false; + if (!status_is_valid(*status)) { + log_message(LOG_LEVEL_ERROR, "Received unknown protocol status %d", *status); + return false; + } if (!protocol_capture_error_detail(session, status, &deadline, NULL)) return false; log_debug_message(LOG_DEBUG_PROTO, "Received Status: %s", status_to_string(*status)); @@ -925,6 +942,10 @@ bool protocol_receive_status_keepalive(ProtocolSession* session, Status* status, Status received; if (!protocol_read_status_until(session, &received, &deadline)) return false; + if (!status_is_valid(received)) { + log_message(LOG_LEVEL_ERROR, "Received unknown protocol status %d", received); + return false; + } if (!protocol_capture_error_detail(session, &received, &deadline, abort_check)) return false; if (received == STATUS_KEEPALIVE) { diff --git a/src/shared/protocol.h b/src/shared/protocol.h index 1ecdab3..73b9132 100644 --- a/src/shared/protocol.h +++ b/src/shared/protocol.h @@ -269,6 +269,9 @@ bool protocol_send_int(ProtocolSession* session, int data); bool protocol_receive_int(ProtocolSession* session, int* data); bool protocol_send_status(ProtocolSession* session, Status status); bool protocol_receive_status(ProtocolSession* session, Status* status); +/* As protocol_receive_status, but with an explicit per-message deadline + * (seconds) instead of the session's configured io_timeout_sec. */ +bool protocol_receive_status_timed(ProtocolSession* session, Status* status, int timeout_sec); bool send_n_data(int file_descriptor, const void* data, size_t data_size); bool receive_n_data(int file_descriptor, void* data, size_t data_size); diff --git a/src/shared/queue.c b/src/shared/queue.c index 3ebac08..2c3de0a 100644 --- a/src/shared/queue.c +++ b/src/shared/queue.c @@ -128,7 +128,7 @@ bool queue_enqueue_multithreaded_cancel(Queue* queue, void* item, mtx_t* mutex, void* queue_dequeue(Queue* queue) { if (queue == NULL || queue_is_empty(queue)) { - log_perror("ERROR: Could not dequeue from null or empty queue."); + log_message(LOG_LEVEL_ERROR, "%s", "ERROR: Could not dequeue from null or empty queue."); return NULL; } @@ -145,7 +145,7 @@ bool queue_push(Queue* queue, void* item) { void* queue_pop(Queue* queue) { if (queue == NULL || queue_is_empty(queue)) { - log_perror("ERROR: Could not pop from null or empty queue."); + log_message(LOG_LEVEL_ERROR, "%s", "ERROR: Could not pop from null or empty queue."); return NULL; } diff --git a/tests/test_protocol.c b/tests/test_protocol.c index f1b3c64..2665096 100644 --- a/tests/test_protocol.c +++ b/tests/test_protocol.c @@ -216,6 +216,38 @@ static void test_send_receive_status() { close(p[1]); } +/* An unknown wire status outside the enum range must be rejected as a protocol + * error instead of being handed to the caller as an unexpected verdict. The + * last known enumerator (STATUS_STATS) must still be accepted, proving the + * validation does not reject legitimate statuses. */ +static void test_receive_status_rejects_unknown() { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + ProtocolSession session; + protocol_session_init(&session, p[0], p[1]); + + Status bogus = (Status)(STATUS_STATS + 1); + EXPECT_EQ_INT((int)write(p[1], &bogus, sizeof(bogus)), (int)sizeof(bogus)); + Status received = STATUS_OK; + EXPECT_FALSE(protocol_receive_status(&session, &received)); + + Status negative = (Status)-1; + EXPECT_EQ_INT((int)write(p[1], &negative, sizeof(negative)), (int)sizeof(negative)); + EXPECT_FALSE(protocol_receive_status(&session, &received)); + + Status top = STATUS_STATS; + EXPECT_EQ_INT((int)write(p[1], &top, sizeof(top)), (int)sizeof(top)); + EXPECT_TRUE(protocol_receive_status(&session, &received)); + EXPECT_EQ_INT((int)received, (int)STATUS_STATS); + + Status timed_bogus = (Status)(STATUS_STATS + 7); + EXPECT_EQ_INT((int)write(p[1], &timed_bogus, sizeof(timed_bogus)), (int)sizeof(timed_bogus)); + EXPECT_FALSE(protocol_receive_status_timed(&session, &received, 5)); + + close(p[0]); + close(p[1]); +} + static void test_receive_n_data_truncated() { int p[2]; EXPECT_EQ_INT(pipe(p), 0); @@ -723,6 +755,7 @@ void test_protocol() { test_send_receive_data(); test_send_receive_int(); test_send_receive_status(); + test_receive_status_rejects_unknown(); test_protocol_session_io_timeout(); test_protocol_server_io_timeout_floor(); test_send_receive_status_timed();