diff --git a/include/crm/common/strings_internal.h b/include/crm/common/strings_internal.h index 94a93d7d118..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, unsigned int max, - 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 aa0a17076ff..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, 0, &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/remote.c b/lib/common/remote.c index cf093b0db1d..a6ac4084e6f 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -8,47 +8,79 @@ */ #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 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)); /*! @@ -213,8 +245,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; } } @@ -258,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); @@ -270,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); @@ -297,10 +380,93 @@ 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; } +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 %" PRIu32 + " bytes", header->payload_compressed, header->payload_uncompressed); + + 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) { + 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); + + /* 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); + remote->buffer = uncompressed; + return pcmk_rc_ok; +} + /*! * \internal * \brief Obtain the XML from the currently buffered remote connection message @@ -322,85 +488,23 @@ 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->version > REMOTE_MSG_VERSION) { + pcmk__err("Header version %" PRIu32 " does not match expected version %d", + header->version, REMOTE_MSG_VERSION); + return NULL; + } + if (header->payload_compressed != 0) { - int rc = 0; - unsigned int size_u = 0; - char *uncompressed = NULL; - size_t buffer_size = 0; + int rc = handle_compressed_payload(header, remote); -#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); + if (rc != pcmk_rc_ok) { 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. + /* handle_compressed_payload copies the header to the front of the + * uncompressed buffer, so update the pointer here. */ -#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); - - 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); - 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); + header = (struct remote_header_v0 *) remote->buffer; } /* take ownership of the buffer */ @@ -423,13 +527,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]"); } @@ -876,7 +974,7 @@ connect_socket_once(int sock, const struct sockaddr *addr, socklen_t addrlen) return rc; } - return pcmk_ok; + return pcmk_rc_ok; } /*! diff --git a/lib/common/strings.c b/lib/common/strings.c index 72ea9f02213..4085e0d9b4f 100644 --- a/lib/common/strings.c +++ b/lib/common/strings.c @@ -585,37 +585,51 @@ 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, size_t length, char **result, size_t *result_len) { - int rc; + 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 = strdup(data); + char *uncompressed = NULL; #ifdef CLOCK_MONOTONIC struct timespec after_t; struct timespec before_t; #endif - if (max == 0) { - max = (length * 1.01) + 601; // Size guaranteed to hold result + /* 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); @@ -630,13 +644,11 @@ pcmk__compress(const char *data, unsigned int length, unsigned int max, #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 85994b7a498..93ddcb5c775 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. * @@ -23,25 +23,16 @@ 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, 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)); - unsigned int len; + size_t len; pcmk__assert_exits( CRM_EX_OSERR, @@ -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))