From 1d83df2fa46cc451a8789b0a44bcc3371b1ede00 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 8 May 2026 09:41:44 -0400 Subject: [PATCH 01/12] Refactor: include: Explain what the fields in remote_header_v0 do. --- lib/common/remote.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index cf093b0db1d..2b116ec5902 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -38,17 +38,44 @@ #define ENDIAN_LOCAL 0xBADADBBD struct remote_header_v0 { - uint32_t endian; /* Detect messages from hosts with different endian-ness */ + /*! + * Magic number used to detect whether the message came from a host with + * different endianness than ours + */ + uint32_t endian; + + //! Remote protocol version uint32_t version; + + //! Message ID number uint64_t id; + + //! Currently unused uint64_t flags; + + /*! + * Amount that will be transmitted: + * - Header size plus payload_compressed if the payload is compressed + * - Header size plus payload_uncompressed otherwise + */ uint32_t size_total; + + /*! + * Where the payload starts - should be immediately after this header, + * so sizeof(struct remote_header_v0) + */ uint32_t payload_offset; + + //! If non-zero, the payload is compressed and this is its compressed size uint32_t payload_compressed; - uint32_t payload_uncompressed; - /* New fields get added here */ + /*! + * The payload's size including a terminating null byte. If the payload + * is compressed, this is its size after being decompressed. + */ + uint32_t payload_uncompressed; + /* New fields get added here */ } __attribute__ ((packed)); /*! From f685e631f828f0fe1b8d9e350d352257af816b8b Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 12 Jun 2026 13:49:21 -0400 Subject: [PATCH 02/12] Refactor: libcrmcommon: Add handle_compressed_payload. This just splits the code that handles the possibility of a compressed payload out of pcmk__remote_message_xml and into its own function. Also return a standard pacemaker return code. --- lib/common/remote.c | 160 ++++++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 74 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index 2b116ec5902..78f127a2b55 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -328,6 +328,90 @@ pcmk__remote_send_xml(pcmk__remote_t *remote, const xmlNode *msg) return rc; } +static int +handle_compressed_payload(struct remote_header_v0 *header, + pcmk__remote_t *remote) +{ + int rc = 0; + unsigned int size_u = 0; + char *uncompressed = NULL; + size_t buffer_size = 0; + +#if (UINT32_MAX < UINT_MAX) + if (header->payload_uncompressed >= UINT_MAX) { + pcmk__err("Couldn't decompress message because uncompressed " + "payload size (%" PRIu32 ") is greater than UINT_MAX " + "(%u)", header->payload_uncompressed, UINT_MAX); + return EMSGSIZE; + } +#endif + + /* @TODO Is the extra byte for the null terminator? + * pcmk__remote_send_xml() also adds one byte to the iov length. + * (However, we do need to account for the possibility of receiving a + * message from an untrusted sender.) + */ + size_u = 1 + header->payload_uncompressed; + + /* Header and uncompressed payload must fit in the destination buffer. + * We do not need to separately check the header size here since + * localized_remote_header will return NULL if it's incorrect. + */ +#if (UINT_MAX >= SIZE_MAX) + if ((size_u >= SIZE_MAX) + || (header->payload_offset > (SIZE_MAX - size_u))) { +#else + if (header->payload_offset > (SIZE_MAX - size_u)) { +#endif + pcmk__err("Couldn't decompress message because the required buffer " + "size (%" PRIu32 " + %u) is greater than SIZE_MAX (%zu)", + header->payload_offset, size_u, SIZE_MAX); + return EMSGSIZE; + } + + buffer_size = (size_t) header->payload_offset + size_u; + if (buffer_size > PCMK__REMOTE_MSG_MAX_SIZE) { + pcmk__err("Message size %zu is larger than max allowed %u bytes", + buffer_size, PCMK__REMOTE_MSG_MAX_SIZE); + return EMSGSIZE; + } + + pcmk__trace("Decompressing message data %" PRIu32 " bytes into %u " + "bytes", header->payload_compressed, size_u); + + uncompressed = pcmk__assert_alloc(buffer_size, sizeof(char)); + + rc = BZ2_bzBuffToBuffDecompress(uncompressed + header->payload_offset, + &size_u, + remote->buffer + header->payload_offset, + header->payload_compressed, 1, 0); + rc = pcmk__bzlib2rc(rc); + + if (rc != pcmk_rc_ok && header->version > REMOTE_MSG_VERSION) { + pcmk__warn("Couldn't decompress v%d message, we only understand " + "v%d", + header->version, REMOTE_MSG_VERSION); + free(uncompressed); + return EPROTO; + + } else if (rc != pcmk_rc_ok) { + pcmk__err("Decompression failed: %s " QB_XS " rc=%d", + pcmk_rc_str(rc), rc); + free(uncompressed); + return rc; + } + + pcmk__assert(size_u == header->payload_uncompressed); + + memcpy(uncompressed, remote->buffer, header->payload_offset); /* Preserve the header */ + remote->buffer_size = header->payload_offset + size_u; + + free(remote->buffer); + remote->buffer = uncompressed; + header = localized_remote_header(remote); + return pcmk_rc_ok; +} + /*! * \internal * \brief Obtain the XML from the currently buffered remote connection message @@ -351,83 +435,11 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) /* Support compression on the receiving end now, in case we ever want to add it later */ if (header->payload_compressed != 0) { - int rc = 0; - unsigned int size_u = 0; - char *uncompressed = NULL; - size_t buffer_size = 0; - -#if (UINT32_MAX < UINT_MAX) - if (header->payload_uncompressed >= UINT_MAX) { - pcmk__err("Couldn't decompress message because uncompressed " - "payload size (%" PRIu32 ") is greater than UINT_MAX " - "(%u)", header->payload_uncompressed, UINT_MAX); - return NULL; - } -#endif - - /* @TODO Is the extra byte for the null terminator? - * pcmk__remote_send_xml() also adds one byte to the iov length. - * (However, we do need to account for the possibility of receiving a - * message from an untrusted sender.) - */ - size_u = 1 + header->payload_uncompressed; - - /* Header and uncompressed payload must fit in the destination buffer. - * We do not need to separately check the header size here since - * localized_remote_header will return NULL if it's incorrect. - */ -#if (UINT_MAX >= SIZE_MAX) - if ((size_u >= SIZE_MAX) - || (header->payload_offset > (SIZE_MAX - size_u))) { -#else - if (header->payload_offset > (SIZE_MAX - size_u)) { -#endif - pcmk__err("Couldn't decompress message because the required buffer " - "size (%" PRIu32 " + %u) is greater than SIZE_MAX (%zu)", - header->payload_offset, size_u, SIZE_MAX); - return NULL; - } - - buffer_size = (size_t) header->payload_offset + size_u; - if (buffer_size > PCMK__REMOTE_MSG_MAX_SIZE) { - pcmk__err("Message size %zu is larger than max allowed %u bytes", - buffer_size, PCMK__REMOTE_MSG_MAX_SIZE); - return NULL; - } - - pcmk__trace("Decompressing message data %" PRIu32 " bytes into %u " - "bytes", header->payload_compressed, size_u); - - uncompressed = pcmk__assert_alloc(buffer_size, sizeof(char)); - - rc = BZ2_bzBuffToBuffDecompress(uncompressed + header->payload_offset, - &size_u, - remote->buffer + header->payload_offset, - header->payload_compressed, 1, 0); - rc = pcmk__bzlib2rc(rc); + int rc = handle_compressed_payload(header, remote); - if (rc != pcmk_rc_ok && header->version > REMOTE_MSG_VERSION) { - pcmk__warn("Couldn't decompress v%d message, we only understand " - "v%d", - header->version, REMOTE_MSG_VERSION); - free(uncompressed); - return NULL; - - } else if (rc != pcmk_rc_ok) { - pcmk__err("Decompression failed: %s " QB_XS " rc=%d", - pcmk_rc_str(rc), rc); - free(uncompressed); + if (rc != pcmk_rc_ok) { return NULL; } - - pcmk__assert(size_u == header->payload_uncompressed); - - memcpy(uncompressed, remote->buffer, header->payload_offset); /* Preserve the header */ - remote->buffer_size = header->payload_offset + size_u; - - free(remote->buffer); - remote->buffer = uncompressed; - header = localized_remote_header(remote); } /* take ownership of the buffer */ From 7aa8a4698a8d309f26967fc728bb5b75941dac51 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 8 May 2026 10:35:30 -0400 Subject: [PATCH 03/12] Refactor: libcrmcommon: Improve remote message version checking. * Perform this check before even looking at the payload. * The error message doesn't need to refer to being able to decompress the payload. It doesn't have anything to do with that. * Remove a redundant check that happens later on during payload XML parsing. --- lib/common/remote.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index 78f127a2b55..03523a5560d 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -387,14 +387,7 @@ handle_compressed_payload(struct remote_header_v0 *header, header->payload_compressed, 1, 0); rc = pcmk__bzlib2rc(rc); - if (rc != pcmk_rc_ok && header->version > REMOTE_MSG_VERSION) { - pcmk__warn("Couldn't decompress v%d message, we only understand " - "v%d", - header->version, REMOTE_MSG_VERSION); - free(uncompressed); - return EPROTO; - - } else if (rc != pcmk_rc_ok) { + if (rc != pcmk_rc_ok) { pcmk__err("Decompression failed: %s " QB_XS " rc=%d", pcmk_rc_str(rc), rc); free(uncompressed); @@ -433,6 +426,12 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) return NULL; } + if (header->version > REMOTE_MSG_VERSION) { + pcmk__err("Header version %" PRIu32 " does not match expected version %d", + header->version, REMOTE_MSG_VERSION); + return NULL; + } + /* Support compression on the receiving end now, in case we ever want to add it later */ if (header->payload_compressed != 0) { int rc = handle_compressed_payload(header, remote); @@ -462,13 +461,7 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) xml = pcmk__xml_parse(payload); if (xml == NULL) { - if (header->version > REMOTE_MSG_VERSION) { - pcmk__warn("Couldn't parse v%d message, we only understand v%d", - header->version, REMOTE_MSG_VERSION); - } else { - pcmk__err("Couldn't parse: '%.120s'", payload); - } - + pcmk__err("Couldn't parse: '%.120s'", payload); } else { pcmk__log_xml_trace(xml, "[remote msg]"); } From dfcf497541ec7bb58a4db7e116387e4dea491d7b Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 8 May 2026 11:07:39 -0400 Subject: [PATCH 04/12] Refactor: libcrmcommon: Remove a redundant call to localized_remote_header. The first thing pcmk__remote_message_xml does is call localized_remote_header to check and extract the header from the incoming message. It then calls handle_compressed_payload which copies that header to the beginning of the uncompressed buffer and then appends the uncompressed payload. Calling that function again at the end of handle_compressed_payload seems to be redundant. All the byte-swapping assignments and checks have already happened on this same data. We do, however, want to update the header pointer in the caller so everything's pointing at the same data. --- lib/common/remote.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index 03523a5560d..28698d15ccb 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -401,7 +401,6 @@ handle_compressed_payload(struct remote_header_v0 *header, free(remote->buffer); remote->buffer = uncompressed; - header = localized_remote_header(remote); return pcmk_rc_ok; } @@ -439,6 +438,11 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) if (rc != pcmk_rc_ok) { return NULL; } + + /* handle_compressed_payload copies the header to the front of the + * uncompressed buffer, so update the pointer here. + */ + header = (struct remote_header_v0 *) remote->buffer; } /* take ownership of the buffer */ From 1610ca05d559c9a31fcbe80842ece05c83bd776f Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 12 Jun 2026 13:51:38 -0400 Subject: [PATCH 05/12] Refactor: libcrmcommon: Minor reformatting in handle_compressed_payload. --- lib/common/remote.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index 28698d15ccb..db64397b011 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -388,15 +388,16 @@ handle_compressed_payload(struct remote_header_v0 *header, rc = pcmk__bzlib2rc(rc); if (rc != pcmk_rc_ok) { - pcmk__err("Decompression failed: %s " QB_XS " rc=%d", - pcmk_rc_str(rc), rc); + pcmk__err("Decompression failed: %s " QB_XS " rc=%d", pcmk_rc_str(rc), + rc); free(uncompressed); return rc; } pcmk__assert(size_u == header->payload_uncompressed); - memcpy(uncompressed, remote->buffer, header->payload_offset); /* Preserve the header */ + /* Copy the header to the front of the uncompressed buffer */ + memcpy(uncompressed, remote->buffer, header->payload_offset); remote->buffer_size = header->payload_offset + size_u; free(remote->buffer); From c2f253698d19dcdc9058f3e63843294b0df8ab1d Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 8 May 2026 16:56:51 -0400 Subject: [PATCH 06/12] Refactor: libcrmcommon: Don't print the unsent part of a message. Even though it's sent plain text, it might still be compressed and therefore printing it out will just look like garbage. --- lib/common/remote.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index db64397b011..bd8afcf407f 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -240,8 +240,7 @@ send_plaintext(int sock, struct iovec *iov) unsent_len -= write_rc; } else { - pcmk__trace("Sent all %zd bytes remaining: %.100s", write_rc, - (const char *) iov->iov_base); + pcmk__trace("Sent all %zd bytes", write_rc); return pcmk_rc_ok; } } From 1557b95540abf1f557d19d859ced32ab5016a3de Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 26 Jun 2026 11:06:57 -0400 Subject: [PATCH 07/12] Refactor: libcrmcommon: Clean up header block in remote.c. --- lib/common/remote.c | 55 ++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index bd8afcf407f..a7b7ed19f5a 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -8,31 +8,36 @@ */ #include -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include // PRIu32, PRIx32 - -#include -#include - -#include -#include - -#include + +#include // htons, inet_ntop +#include // errno, EAGAIN, EINTR, EINVAL +#include // PRIu32, uint32_t, SIZE_MAX +#include // UINT_MAX +#include // addrinfo, freeaddrinfo, getaddrinfo +#include // INET6_ADDRSTRLEN, sockaddr_in +#include // TCP_USER_TIMEOUT, SOL_TCP +#include // pollfd, poll, POLLIN +#include // true, bool, false +#include // NULL, free, size_t, strtol +#include // memset, memcpy, strcpy +#include // FD_ISSET, fd_set, select +#include // connect, socklen_t, AF_INET6 +#include // timeval +#include // ssize_t, time_t +#include // iovec +#include // time +#include // close, read, write + +#include // BZ2_bzBuffToBuffDecompress +#include // GUINT32_SWAP_LE_BE, g_string_* +#include // gnutls_strerror, GNUTLS_E_AGAIN +#include // xmlNode +#include // QB_XS + +#include // CRM_CHECK +#include // pcmk_rc_* +#include // crm_default_remote_port +#include // pcmk__xml_* #define REMOTE_MSG_VERSION 1 #define ENDIAN_LOCAL 0xBADADBBD From a18956e9943ed622ef537e3c1bdc6b52183f5c96 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 26 Jun 2026 11:07:17 -0400 Subject: [PATCH 08/12] Refactor: libcrmcommon: Use pcmk_rc_ok in connect_socket_once. --- lib/common/remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index a7b7ed19f5a..db6b4c31ef2 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -917,7 +917,7 @@ connect_socket_once(int sock, const struct sockaddr *addr, socklen_t addrlen) return rc; } - return pcmk_ok; + return pcmk_rc_ok; } /*! From b1ac27a36a2304957b4fe335841cd5515ca1d15d Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 26 Jun 2026 11:57:54 -0400 Subject: [PATCH 09/12] Refactor: libcrmcommon: Remove the max argument from pcmk__compress. Outside of testing, it's always 0. --- include/crm/common/strings_internal.h | 4 ++-- lib/cluster/cpg.c | 2 +- lib/common/strings.c | 10 +++------- lib/common/tests/strings/pcmk__compress_test.c | 16 +++------------- 4 files changed, 9 insertions(+), 23 deletions(-) diff --git a/include/crm/common/strings_internal.h b/include/crm/common/strings_internal.h index 94a93d7d118..36efdbf1044 100644 --- a/include/crm/common/strings_internal.h +++ b/include/crm/common/strings_internal.h @@ -47,8 +47,8 @@ int pcmk__uint_from_hash(GHashTable *table, const char *key, unsigned int default_val, unsigned int *result); void pcmk__add_separated_word(GString **list, size_t init_size, const char *word, const char *separator); -int pcmk__compress(const char *data, unsigned int length, unsigned int max, - char **result, unsigned int *result_len); +int pcmk__compress(const char *data, unsigned int length, char **result, + unsigned int *result_len); int pcmk__scan_ll(const char *text, long long *result, long long default_value); int pcmk__scan_min_int(const char *text, int *result, int minimum); diff --git a/lib/cluster/cpg.c b/lib/cluster/cpg.c index aa0a17076ff..598578335aa 100644 --- a/lib/cluster/cpg.c +++ b/lib/cluster/cpg.c @@ -984,7 +984,7 @@ send_cpg_text(const char *data, const pcmk__node_status_t *node, char *compressed = NULL; unsigned int new_size = 0; - if (pcmk__compress(data, (unsigned int) msg->size, 0, &compressed, + if (pcmk__compress(data, (unsigned int) msg->size, &compressed, &new_size) == pcmk_rc_ok) { msg->header.size = sizeof(pcmk__cpg_msg_t) + new_size; diff --git a/lib/common/strings.c b/lib/common/strings.c index 72ea9f02213..870404ff925 100644 --- a/lib/common/strings.c +++ b/lib/common/strings.c @@ -585,16 +585,16 @@ pcmk__add_separated_word(GString **list, size_t init_size, const char *word, * * \param[in] data Data to compress * \param[in] length Number of characters of data to compress - * \param[in] max Maximum size of compressed data (or 0 to estimate) * \param[out] result Where to store newly allocated compressed result * \param[out] result_len Where to store actual compressed length of result * * \return Standard Pacemaker return code */ int -pcmk__compress(const char *data, unsigned int length, unsigned int max, - char **result, unsigned int *result_len) +pcmk__compress(const char *data, unsigned int length, char **result, + unsigned int *result_len) { + unsigned int max = (length * 1.01) + 601; // Size guaranteed to hold result int rc; char *compressed = NULL; char *uncompressed = strdup(data); @@ -603,10 +603,6 @@ pcmk__compress(const char *data, unsigned int length, unsigned int max, struct timespec before_t; #endif - if (max == 0) { - max = (length * 1.01) + 601; // Size guaranteed to hold result - } - #ifdef CLOCK_MONOTONIC clock_gettime(CLOCK_MONOTONIC, &before_t); #endif diff --git a/lib/common/tests/strings/pcmk__compress_test.c b/lib/common/tests/strings/pcmk__compress_test.c index 85994b7a498..f76e2e85cf6 100644 --- a/lib/common/tests/strings/pcmk__compress_test.c +++ b/lib/common/tests/strings/pcmk__compress_test.c @@ -1,5 +1,5 @@ /* - * Copyright 2022-2025 the Pacemaker project contributors + * Copyright 2022-2026 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -25,19 +25,10 @@ simple_compress(void **state) char *result = pcmk__assert_alloc(1024, sizeof(char)); unsigned int len; - assert_int_equal(pcmk__compress(SIMPLE_DATA, 40, 0, &result, &len), pcmk_rc_ok); + assert_int_equal(pcmk__compress(SIMPLE_DATA, 40, &result, &len), pcmk_rc_ok); assert_memory_equal(result, SIMPLE_COMPRESSED, 13); } -static void -max_too_small(void **state) -{ - char *result = pcmk__assert_alloc(1024, sizeof(char)); - unsigned int len; - - assert_int_equal(pcmk__compress(SIMPLE_DATA, 40, 10, &result, &len), EFBIG); -} - static void calloc_fails(void **state) { char *result = pcmk__assert_alloc(1024, sizeof(char)); @@ -50,7 +41,7 @@ calloc_fails(void **state) { expect_uint_value(__wrap_calloc, nmemb, (size_t) ((40 * 1.01) + 601)); expect_uint_value(__wrap_calloc, size, sizeof(char)); - pcmk__compress(SIMPLE_DATA, 40, 0, &result, &len); + pcmk__compress(SIMPLE_DATA, 40, &result, &len); pcmk__mock_calloc = false; // Use the real calloc() } ); @@ -58,5 +49,4 @@ calloc_fails(void **state) { PCMK__UNIT_TEST(NULL, NULL, cmocka_unit_test(simple_compress), - cmocka_unit_test(max_too_small), cmocka_unit_test(calloc_fails)) From 3fc0f38fcdcd3d00a904beb45dcbd4168c347d7e Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 26 Jun 2026 12:15:33 -0400 Subject: [PATCH 10/12] Refactor: libcrmcommon: Don't strdup in pcmk__compress until later. --- lib/common/strings.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/common/strings.c b/lib/common/strings.c index 870404ff925..752b4c52794 100644 --- a/lib/common/strings.c +++ b/lib/common/strings.c @@ -595,9 +595,9 @@ pcmk__compress(const char *data, unsigned int length, char **result, unsigned int *result_len) { unsigned int max = (length * 1.01) + 601; // Size guaranteed to hold result - int rc; + int rc = pcmk_rc_ok; char *compressed = NULL; - char *uncompressed = strdup(data); + char *uncompressed = NULL; #ifdef CLOCK_MONOTONIC struct timespec after_t; struct timespec before_t; @@ -608,6 +608,7 @@ pcmk__compress(const char *data, unsigned int length, char **result, #endif compressed = pcmk__assert_alloc((size_t) max, sizeof(char)); + uncompressed = strdup(data); *result_len = max; rc = BZ2_bzBuffToBuffCompress(compressed, result_len, uncompressed, length, From 4ff4eba96fcf3fd738246f2fa61a341bbc1bae03 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 26 Jun 2026 12:54:56 -0400 Subject: [PATCH 11/12] Refactor: libcrmcommon: pcmk__compress now takes size_t arguments. The idea here is that we can consolidate some of the integer overflow checking into this function, making its callers less complicated. Note that send_cpg_text needs a lot of the same checking that's been added to pcmk__remote_send_xml recently, though we don't need to be quite as defensive in that function. We don't allow just any client to connect to the cluster layer, so the potential for bad actors should be considerably lower. For the moment I'm just making sure the function continues to compile. Also note that send_cpg_text never returns false indicating that sending failed. There's probably a lot of error handling that could be done. --- include/crm/common/strings_internal.h | 4 +-- lib/cluster/cpg.c | 6 ++-- lib/common/strings.c | 35 +++++++++++++------ .../tests/strings/pcmk__compress_test.c | 4 +-- 4 files changed, 31 insertions(+), 18 deletions(-) diff --git a/include/crm/common/strings_internal.h b/include/crm/common/strings_internal.h index 36efdbf1044..81ddddb805e 100644 --- a/include/crm/common/strings_internal.h +++ b/include/crm/common/strings_internal.h @@ -47,8 +47,8 @@ int pcmk__uint_from_hash(GHashTable *table, const char *key, unsigned int default_val, unsigned int *result); void pcmk__add_separated_word(GString **list, size_t init_size, const char *word, const char *separator); -int pcmk__compress(const char *data, unsigned int length, char **result, - unsigned int *result_len); +int pcmk__compress(const char *data, size_t length, char **result, + size_t *result_len); int pcmk__scan_ll(const char *text, long long *result, long long default_value); int pcmk__scan_min_int(const char *text, int *result, int minimum); diff --git a/lib/cluster/cpg.c b/lib/cluster/cpg.c index 598578335aa..1ab23e5d499 100644 --- a/lib/cluster/cpg.c +++ b/lib/cluster/cpg.c @@ -982,11 +982,9 @@ send_cpg_text(const char *data, const pcmk__node_status_t *node, } else { char *compressed = NULL; - unsigned int new_size = 0; - - if (pcmk__compress(data, (unsigned int) msg->size, &compressed, - &new_size) == pcmk_rc_ok) { + size_t new_size = 0; + if (pcmk__compress(data, msg->size, &compressed, &new_size) == pcmk_rc_ok) { msg->header.size = sizeof(pcmk__cpg_msg_t) + new_size; msg = pcmk__realloc(msg, msg->header.size); memcpy(msg->data, compressed, new_size); diff --git a/lib/common/strings.c b/lib/common/strings.c index 752b4c52794..4085e0d9b4f 100644 --- a/lib/common/strings.c +++ b/lib/common/strings.c @@ -591,10 +591,10 @@ pcmk__add_separated_word(GString **list, size_t init_size, const char *word, * \return Standard Pacemaker return code */ int -pcmk__compress(const char *data, unsigned int length, char **result, - unsigned int *result_len) +pcmk__compress(const char *data, size_t length, char **result, size_t *result_len) { - unsigned int max = (length * 1.01) + 601; // Size guaranteed to hold result + size_t max = (length * 1.01) + 601; // Max size of the compressed result + unsigned int dest_len = 0; // Where bz2 should store the actual size int rc = pcmk_rc_ok; char *compressed = NULL; char *uncompressed = NULL; @@ -603,16 +603,33 @@ pcmk__compress(const char *data, unsigned int length, char **result, struct timespec before_t; #endif + /* Did the max calculation overflow? */ + if (max < length) { + return EINVAL; + } + + /* BZ2_bzBuffToBuffCompress wants unsigned ints, not size_t. This function + * takes size_t arguments to simplify checking in its callers. Make sure + * we're not passing BZ2_bzBuffToBuffCompress something that's too large. + */ +#if (SIZE_MAX > UINT_MAX) + if (max > UINT_MAX) { + return EINVAL; + } +#endif + #ifdef CLOCK_MONOTONIC clock_gettime(CLOCK_MONOTONIC, &before_t); #endif - compressed = pcmk__assert_alloc((size_t) max, sizeof(char)); + compressed = pcmk__assert_alloc(max, sizeof(char)); uncompressed = strdup(data); - *result_len = max; - rc = BZ2_bzBuffToBuffCompress(compressed, result_len, uncompressed, length, + dest_len = max; + rc = BZ2_bzBuffToBuffCompress(compressed, &dest_len, uncompressed, length, PCMK__BZ2_BLOCKS, 0, PCMK__BZ2_WORK); + *result_len = dest_len; + rc = pcmk__bzlib2rc(rc); free(uncompressed); @@ -627,13 +644,11 @@ pcmk__compress(const char *data, unsigned int length, char **result, #ifdef CLOCK_MONOTONIC clock_gettime(CLOCK_MONOTONIC, &after_t); - pcmk__trace("Compressed %d bytes into %d (ratio %d:1) in %.0fms", length, - *result_len, (length / *result_len), + pcmk__trace("Compressed %zu bytes into %zu in %.0fms", length, *result_len, (((after_t.tv_sec - before_t.tv_sec) * 1000) + ((after_t.tv_nsec - before_t.tv_nsec) / 1e6))); #else - pcmk__trace("Compressed %d bytes into %d (ratio %d:1)", length, *result_len, - (length / *result_len)); + pcmk__trace("Compressed %zu bytes into %zu", length, *result_len); #endif *result = compressed; diff --git a/lib/common/tests/strings/pcmk__compress_test.c b/lib/common/tests/strings/pcmk__compress_test.c index f76e2e85cf6..93ddcb5c775 100644 --- a/lib/common/tests/strings/pcmk__compress_test.c +++ b/lib/common/tests/strings/pcmk__compress_test.c @@ -23,7 +23,7 @@ static void simple_compress(void **state) { char *result = pcmk__assert_alloc(1024, sizeof(char)); - unsigned int len; + size_t len; assert_int_equal(pcmk__compress(SIMPLE_DATA, 40, &result, &len), pcmk_rc_ok); assert_memory_equal(result, SIMPLE_COMPRESSED, 13); @@ -32,7 +32,7 @@ simple_compress(void **state) static void calloc_fails(void **state) { char *result = pcmk__assert_alloc(1024, sizeof(char)); - unsigned int len; + size_t len; pcmk__assert_exits( CRM_EX_OSERR, From 83c7f4aa94867fbd1b6cb90453bd398e31490d4a Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 12 Jun 2026 14:23:35 -0400 Subject: [PATCH 12/12] Feature: libcrmcommon: Add support for compression to remote messages. This has been enabled on the receive side of remote connections since 2013 just in case we ever want to use it. However, because we've never sent compressed messages, nothing ever used this code and so it's developed various bugs. I think it's better if we just enable this code instead of carrying around unused and buggy code. There's not really much to do in pcmk__remote_send_xml. We just need to make sure that the message is big enough to make compression worth it, do the compression, and make sure all the sizes are correct in the message header. We also need to be careful to free things at the right time and with the right function given that pcmk__compress allocates memory for the compressed version. Two additional notes: * I've chosen PCMK__BZ2_THRESHOLD as the definition of "big enough" because we are using this elsewhere for the same purpose, but we could experiment with different values here. I do not want to make this a user-exposed tuneable. * You may remember that I previously removed compression support for IPC messages in favor of multi-part messages. Adding compression support here might seem a little inconsistent, but I'm unsure that we can actually do that and still retain backwards compatibility. At the least, we would have to bump the remote message protocol and probably change the header format which would make it difficult to support older remote systems. For now at least, using the compression code we essentially already supported is easiest. --- lib/common/remote.c | 77 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index db6b4c31ef2..a6ac4084e6f 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -289,6 +289,8 @@ pcmk__remote_send_xml(pcmk__remote_t *remote, const xmlNode *msg) struct iovec iov[2]; struct remote_header_v0 *header; + bool did_compress = false; + size_t payload_len = 0; CRM_CHECK((remote != NULL) && (msg != NULL), return EINVAL); @@ -301,23 +303,73 @@ pcmk__remote_send_xml(pcmk__remote_t *remote, const xmlNode *msg) iov[0].iov_base = header; iov[0].iov_len = sizeof(struct remote_header_v0); - - iov[1].iov_len = 1 + xml_text->len; - iov[1].iov_base = g_string_free(xml_text, FALSE); + iov[1].iov_base = NULL; + iov[1].iov_len = 0; id++; header->id = id; header->endian = ENDIAN_LOCAL; header->version = REMOTE_MSG_VERSION; header->payload_offset = iov[0].iov_len; - header->payload_uncompressed = iov[1].iov_len; - if ((UINT32_MAX - iov[0].iov_len) < iov[1].iov_len) { + payload_len = 1 + xml_text->len; + + /* Check that: + * - the above addition calculating payload_len did not overflow + * - payload_len + header size fits in a uint32_t (therefore, payload_len + * will also fit into a uint32_t when we assign it to payload_uncompressed + * later) + */ + if ((payload_len < xml_text->len) || (UINT32_MAX - iov[0].iov_len) < payload_len) { pcmk__err("Remote message size %zu + %zu exceeds maximum of %" PRIu32, - iov[0].iov_len, iov[1].iov_len, UINT32_MAX); + iov[0].iov_len, payload_len, UINT32_MAX); goto done; } + if (payload_len >= PCMK__BZ2_THRESHOLD) { + char *compressed = NULL; + size_t new_size = 0; + + if (pcmk__compress(xml_text->str, payload_len, &compressed, + &new_size) == pcmk_rc_ok) { + + /* The above call might have given us a new_size value that's + * different from the original payload_len, so we need to repeat the + * overflow check. + */ +#if (SIZE_MAX > UINT32_MAX) + if ((new_size > UINT32_MAX) + || ((UINT32_MAX - iov[0].iov_len) < new_size)) { +#else + if ((UINT32_MAX - iov[0].iov_len) < new_size) { +#endif + pcmk__err("Compressed message size %zu exceeds maximum of %" PRIu32, + new_size, UINT32_MAX); + rc = EMSGSIZE; + + g_string_free(xml_text, TRUE); + free(compressed); + goto done; + } + + iov[1].iov_len = new_size; + iov[1].iov_base = compressed; + + header->payload_compressed = new_size; + header->payload_uncompressed = payload_len; + g_string_free(xml_text, TRUE); + + did_compress = true; + } + } + + if (!did_compress) { + iov[1].iov_len = payload_len; + iov[1].iov_base = g_string_free(xml_text, FALSE); + + header->payload_uncompressed = iov[1].iov_len; + } + header->size_total = iov[0].iov_len + iov[1].iov_len; rc = remote_send_iovs(remote, iov, 2); @@ -328,7 +380,13 @@ pcmk__remote_send_xml(pcmk__remote_t *remote, const xmlNode *msg) done: free(iov[0].iov_base); - g_free((gchar *) iov[1].iov_base); + + if (did_compress) { + free(iov[1].iov_base); + } else { + g_free(iov[1].iov_base); + } + return rc; } @@ -380,8 +438,8 @@ handle_compressed_payload(struct remote_header_v0 *header, return EMSGSIZE; } - pcmk__trace("Decompressing message data %" PRIu32 " bytes into %u " - "bytes", header->payload_compressed, size_u); + pcmk__trace("Decompressing message data %" PRIu32 " bytes into %" PRIu32 + " bytes", header->payload_compressed, header->payload_uncompressed); uncompressed = pcmk__assert_alloc(buffer_size, sizeof(char)); @@ -436,7 +494,6 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) return NULL; } - /* Support compression on the receiving end now, in case we ever want to add it later */ if (header->payload_compressed != 0) { int rc = handle_compressed_payload(header, remote);