From 874e25f11b794d0e77a5b559c9f72fe9c5ad644f Mon Sep 17 00:00:00 2001 From: Levi Neely <141506390+lneely@users.noreply.github.com> Date: Wed, 4 Mar 2026 19:02:01 +0100 Subject: [PATCH] Fix pcl-zqv.4.2: protocol parsing and buffer safety in prpc/papi (#348) prpc.c: - on_request: fix total_size computation. The original formula sizeof(uint32_t)+sizeof(uint64_t)+response->length had two bugs: (1) sizeof(uint32_t)+sizeof(uint64_t)=12 but offsetof(rpc_message_t,value)=16 due to struct alignment padding between the uint32_t type and uint64_t length fields; and (2) respond() was storing full message size in response->length, double-counting the header. Fix: use offsetof(rpc_message_t,value)+response->length, consistent with readResponse() in rpcclient.cpp which reads a fixed header_size of offsetof(rpc_message_t,value) bytes then reads msg->length payload bytes. - respond: store payload length only in response->length (value_length+1), not full message size, to match the client protocol expectation. - prpc_init: add null check on malloc return value. - prpc_register: restore old handler table and return -1 if prpc_init fails. papi.c: - papi_result_thread: add missing MAX_API_RESPONSE_SIZE guard (present in papi_result but absent here), preventing server-controlled unbounded malloc. - papi_result, papi_result_thread: add null checks on malloc before passing pointer to psock_readall. - papi_result_async: add MAX_API_RESPONSE_SIZE check and malloc null check on reader->respsize path. - calc_ret_len: add _NEED_DATA(1) guard before ARRAY and HASH while-loop conditions; empty containers with datalen=0 caused out-of-bounds read. Co-authored-by: Levi Neely Co-authored-by: Claude Sonnet 4.6 --- pclsync/papi.c | 24 ++++++++++++++++++++++++ pclsync/prpc.c | 14 ++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/pclsync/papi.c b/pclsync/papi.c index 88ddb45..ced472f 100644 --- a/pclsync/papi.c +++ b/pclsync/papi.c @@ -201,6 +201,7 @@ static ssize_t calc_ret_len(unsigned char **restrict data, int unsigned cnt; cnt = 0; ret = sizeof(binresult); + _NEED_DATA(1); while (**data != RPARAM_END) { r = calc_ret_len(data, datalen, strcnt); if (r == -1) @@ -218,6 +219,7 @@ static ssize_t calc_ret_len(unsigned char **restrict data, int unsigned cnt; cnt = 0; ret = sizeof(binresult); + _NEED_DATA(1); while (**data != RPARAM_END) { r = calc_ret_len(data, datalen, strcnt); if (r == -1) @@ -405,6 +407,10 @@ binresult *papi_result(psock_t *sock) { } data = (unsigned char *)malloc(ressize); + if (!data) { + pdbg_logf(D_ERROR, "Failed to allocate %u bytes for API response", ressize); + return NULL; + } if (pdbg_unlikely(psock_readall(sock, data, ressize) != ressize)) { free(data); @@ -424,7 +430,16 @@ binresult *papi_result_thread(psock_t *sock) { if (pdbg_unlikely(psock_readall_thread( sock, &ressize, sizeof(uint32_t)) != sizeof(uint32_t))) return NULL; + if (ressize > MAX_API_RESPONSE_SIZE) { + pdbg_logf(D_WARNING, "API response size %u exceeds limit %u, rejecting", + ressize, MAX_API_RESPONSE_SIZE); + return NULL; + } data = (unsigned char *)malloc(ressize); + if (!data) { + pdbg_logf(D_ERROR, "Failed to allocate %u bytes for API response", ressize); + return NULL; + } if (pdbg_unlikely(psock_readall_thread(sock, data, ressize) != ressize)) { free(data); @@ -467,8 +482,17 @@ again: reader->state = 1; reader->bytesread = 0; reader->bytestoread = reader->respsize; + if (reader->respsize > MAX_API_RESPONSE_SIZE) { + pdbg_logf(D_WARNING, "API response size %u exceeds limit %u, rejecting", + reader->respsize, MAX_API_RESPONSE_SIZE); + reader->result = NULL; + papi_rdr_alloc(reader); + return ASYNC_RES_READY; + } reader->data = (unsigned char *)malloc(reader->respsize); if (!reader->data) { + pdbg_logf(D_ERROR, "Failed to allocate %u bytes for API response", + reader->respsize); reader->result = NULL; papi_rdr_alloc(reader); return ASYNC_RES_READY; diff --git a/pclsync/prpc.c b/pclsync/prpc.c index 1c0cf89..49ab50b 100644 --- a/pclsync/prpc.c +++ b/pclsync/prpc.c @@ -36,6 +36,7 @@ */ #include +#include #include #include #include @@ -108,7 +109,7 @@ static void on_request(void *lpvParam) { if (request) { respond(request, response); - ssize_t total_size = sizeof(uint32_t) + sizeof(uint64_t) + response->length; + ssize_t total_size = offsetof(rpc_message_t, value) + response->length; ssize_t bytes_written = write(*sockfd, response, total_size); if (bytes_written == -1) { @@ -229,7 +230,7 @@ static void respond(rpc_message_t *request, rpc_message_t *response) { response->value[value_length] = '\0'; pdbg_logf(D_WARNING, "Response message truncated to fit buffer"); } - response->length = sizeof(rpc_message_t) + value_length + 1; + response->length = value_length + 1; } void prpc_main_loop() { @@ -297,6 +298,11 @@ int prpc_register(int cmdid, prpc_handler h) { if (cmdid > (calbacks_lower_band + handlers_size)) { handlers_size = cmdid - calbacks_lower_band + 1; prpc_init(); + if (!handlers) { + handlers = handlers_old; + handlers_size = handlers_size_old; + return -1; + } memcpy(handlers, handlers_old, handlers_size_old * sizeof(prpc_handler)); free(handlers_old); @@ -307,6 +313,10 @@ int prpc_register(int cmdid, prpc_handler h) { void prpc_init() { handlers = (prpc_handler *)malloc(sizeof(prpc_handler) * handlers_size); + if (!handlers) { + pdbg_logf(D_ERROR, "Failed to allocate handler table"); + return; + } memset(handlers, 0, sizeof(prpc_handler) * handlers_size); }