From b3a416bd754fcbd2173d90cabb349281098e366d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 16 Jun 2025 17:06:52 -0700 Subject: [PATCH 01/49] Refactor: libcrmcommon: Avoid snprintf() in time_as_string_common() We don't error-check the return code, and the offset update idiom is painful to look at. It won't hurt us to do a tiny bit more string allocation in the heap for readability. We already do a ton of allocation for XML and Pacemaker data structures. Ref T974 Signed-off-by: Reid Wahl --- lib/common/iso8601.c | 171 ++++++++++++++++++++++--------------------- 1 file changed, 86 insertions(+), 85 deletions(-) diff --git a/lib/common/iso8601.c b/lib/common/iso8601.c index b738c0b2851..ec7ccb324ed 100644 --- a/lib/common/iso8601.c +++ b/lib/common/iso8601.c @@ -479,60 +479,58 @@ crm_time_get_isoweek(const crm_time_t *dt, uint32_t *y, uint32_t *w, * \internal * \brief Print "." to a buffer * - * \param[in] sec Seconds - * \param[in] usec Microseconds (must be of same sign as \p sec and of - * absolute value less than \p QB_TIME_US_IN_SEC) - * \param[in,out] buf Result buffer - * \param[in,out] offset Current offset within \p buf + * \param[in] sec Seconds + * \param[in] usec Microseconds (must be of same sign as \p sec and of + * absolute value less than \c QB_TIME_US_IN_SEC) + * \param[in,out] buf Result buffer */ static inline void -sec_usec_as_string(long long sec, int usec, char *buf, size_t *offset) +sec_usec_as_string(long long sec, int usec, GString *buf) { - *offset += snprintf(buf + *offset, DATE_MAX - *offset, "%s%lld.%06d", - ((sec == 0) && (usec < 0))? "-" : "", - sec, QB_ABS(usec)); + /* A negative value smaller than -1 second should have the negative sign + * before the 0, not before the usec part + */ + if ((sec == 0) && (usec < 0)) { + g_string_append_c(buf, '-'); + } + g_string_append_printf(buf, "%lld.%06d", sec, QB_ABS(usec)); } /*! * \internal * \brief Get a string representation of a duration * - * \param[in] dt Time object to interpret as a duration - * \param[in] usec Microseconds to add to \p dt - * \param[in] show_usec Whether to include microseconds in \p result - * \param[out] result Where to store the result string + * \param[in] dt Time object to interpret as a duration + * \param[in] usec Microseconds to add to \p dt + * \param[in] show_usec Whether to include microseconds in \p buf + * \param[in,out] buf Result buffer */ static void -crm_duration_as_string(const crm_time_t *dt, int usec, bool show_usec, - char *result) +duration_as_string(const crm_time_t *dt, int usec, bool show_usec, GString *buf) { - size_t offset = 0; - pcmk__assert(valid_sec_usec(dt->seconds, usec)); if (dt->years) { - offset += snprintf(result + offset, DATE_MAX - offset, "%4d year%s ", - dt->years, pcmk__plural_s(dt->years)); + g_string_append_printf(buf, "%4d year%s ", + dt->years, pcmk__plural_s(dt->years)); } if (dt->months) { - offset += snprintf(result + offset, DATE_MAX - offset, "%2d month%s ", - dt->months, pcmk__plural_s(dt->months)); + g_string_append_printf(buf, "%2d month%s ", + dt->months, pcmk__plural_s(dt->months)); } if (dt->days) { - offset += snprintf(result + offset, DATE_MAX - offset, "%2d day%s ", - dt->days, pcmk__plural_s(dt->days)); + g_string_append_printf(buf, "%2d day%s ", + dt->days, pcmk__plural_s(dt->days)); } // At least print seconds (and optionally usecs) - if ((offset == 0) || (dt->seconds != 0) || (show_usec && (usec != 0))) { + if ((buf->len == 0) || (dt->seconds != 0) || (show_usec && (usec != 0))) { if (show_usec) { - sec_usec_as_string(dt->seconds, usec, result, &offset); + sec_usec_as_string(dt->seconds, usec, buf); } else { - offset += snprintf(result + offset, DATE_MAX - offset, "%d", - dt->seconds); + g_string_append_printf(buf, "%d", dt->seconds); } - offset += snprintf(result + offset, DATE_MAX - offset, " second%s", - pcmk__plural_s(dt->seconds)); + g_string_append_printf(buf, " second%s", pcmk__plural_s(dt->seconds)); } // More than one minute, so provide a more readable breakdown into units @@ -546,32 +544,37 @@ crm_duration_as_string(const crm_time_t *dt, int usec, bool show_usec, crm_time_get_sec(dt->seconds, &h, &m, &s); print_sec_component = ((s != 0) || (show_usec && (u != 0))); - offset += snprintf(result + offset, DATE_MAX - offset, " ("); + g_string_append(buf, " ("); if (h) { - offset += snprintf(result + offset, DATE_MAX - offset, - "%" PRIu32 " hour%s%s", h, pcmk__plural_s(h), - ((m != 0) || print_sec_component)? " " : ""); + g_string_append_printf(buf, "%" PRIu32 " hour%s", + h, pcmk__plural_s(h)); + + if ((m != 0) || print_sec_component) { + g_string_append_c(buf, ' '); + } } if (m) { - offset += snprintf(result + offset, DATE_MAX - offset, - "%" PRIu32 " minute%s%s", m, pcmk__plural_s(m), - print_sec_component? " " : ""); + g_string_append_printf(buf, "%" PRIu32 " minute%s", + m, pcmk__plural_s(m)); + + if (print_sec_component) { + g_string_append_c(buf, ' '); + } } if (print_sec_component) { if (show_usec) { - sec_usec_as_string(s, u, result, &offset); + sec_usec_as_string(s, u, buf); } else { - offset += snprintf(result + offset, DATE_MAX - offset, - "%" PRIu32, s); + g_string_append_printf(buf, "%" PRIu32, s); } - offset += snprintf(result + offset, DATE_MAX - offset, " second%s", - pcmk__plural_s(dt->seconds)); + g_string_append_printf(buf, " second%s", + pcmk__plural_s(dt->seconds)); } - offset += snprintf(result + offset, DATE_MAX - offset, ")"); + g_string_append_c(buf, ')'); } } @@ -579,35 +582,36 @@ crm_duration_as_string(const crm_time_t *dt, int usec, bool show_usec, * \internal * \brief Get a string representation of a time object * - * \param[in] dt Time to convert to string - * \param[in] usec Microseconds to add to \p dt - * \param[in] flags Group of \p crm_time_* string format options - * \param[out] result Where to store the result string + * \param[in] dt Time to convert to string + * \param[in] usec Microseconds to add to \p dt + * \param[in] flags Group of \c crm_time_* string format options * - * \note \p result must be of size \p DATE_MAX or larger. + * \return Newly allocated string representation of \p dt plus \p usec + * + * \note The caller is responsible for freeing the return value using \c free(). */ -static void -time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, - char *result) +static char * +time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags) { crm_time_t *utc = NULL; - size_t offset = 0; + GString *buf = NULL; + char *result = NULL; if (!crm_time_is_defined(dt)) { - strcpy(result, ""); - return; + return pcmk__str_copy(""); } pcmk__assert(valid_sec_usec(dt->seconds, usec)); + buf = g_string_sized_new(128); + /* Simple cases: as duration, seconds, or seconds since epoch. * These never depend on time zone. */ if (pcmk_is_set(flags, crm_time_log_duration)) { - crm_duration_as_string(dt, usec, pcmk_is_set(flags, crm_time_usecs), - result); - return; + duration_as_string(dt, usec, pcmk_is_set(flags, crm_time_usecs), buf); + goto done; } if (pcmk_any_flags_set(flags, crm_time_seconds|crm_time_epoch)) { @@ -620,11 +624,11 @@ time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, } if (pcmk_is_set(flags, crm_time_usecs)) { - sec_usec_as_string(seconds, usec, result, &offset); + sec_usec_as_string(seconds, usec, buf); } else { - snprintf(result, DATE_MAX, "%lld", seconds); + g_string_append_printf(buf, "%lld", seconds); } - return; + goto done; } // Convert to UTC if local timezone was not requested @@ -643,9 +647,9 @@ time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, uint32_t d = 0; if (crm_time_get_isoweek(dt, &y, &w, &d)) { - offset += snprintf(result + offset, DATE_MAX - offset, - "%" PRIu32 "-W%.2" PRIu32 "-%" PRIu32, - y, w, d); + g_string_append_printf(buf, + "%" PRIu32 "-W%.2" PRIu32 "-%" PRIu32, + y, w, d); } } else if (pcmk_is_set(flags, crm_time_ordinal)) { // YYYY-DDD @@ -653,8 +657,7 @@ time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, uint32_t d = 0; if (crm_time_get_ordinal(dt, &y, &d)) { - offset += snprintf(result + offset, DATE_MAX - offset, - "%" PRIu32 "-%.3" PRIu32, y, d); + g_string_append_printf(buf, "%" PRIu32 "-%.3" PRIu32, y, d); } } else { // YYYY-MM-DD @@ -663,9 +666,9 @@ time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, uint32_t d = 0; if (crm_time_get_gregorian(dt, &y, &m, &d)) { - offset += snprintf(result + offset, DATE_MAX - offset, - "%.4" PRIu32 "-%.2" PRIu32 "-%.2" PRIu32, - y, m, d); + g_string_append_printf(buf, + "%.4" PRIu32 "-%.2" PRIu32 "-%.2" PRIu32, + y, m, d); } } } @@ -673,33 +676,36 @@ time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, if (pcmk_is_set(flags, crm_time_log_timeofday)) { uint32_t h = 0, m = 0, s = 0; - if (offset > 0) { - offset += snprintf(result + offset, DATE_MAX - offset, " "); + if (buf->len > 0) { + g_string_append_c(buf, ' '); } if (crm_time_get_timeofday(dt, &h, &m, &s)) { - offset += snprintf(result + offset, DATE_MAX - offset, - "%.2" PRIu32 ":%.2" PRIu32 ":%.2" PRIu32, - h, m, s); + g_string_append_printf(buf, + "%.2" PRIu32 ":%.2" PRIu32 ":%.2" PRIu32, + h, m, s); if (pcmk_is_set(flags, crm_time_usecs)) { - offset += snprintf(result + offset, DATE_MAX - offset, - ".%06" PRIu32, QB_ABS(usec)); + g_string_append_printf(buf, ".%06" PRIu32, QB_ABS(usec)); } } if (pcmk_is_set(flags, crm_time_log_with_timezone) && (dt->offset != 0)) { crm_time_get_sec(dt->offset, &h, &m, &s); - offset += snprintf(result + offset, DATE_MAX - offset, - " %c%.2" PRIu32 ":%.2" PRIu32, - ((dt->offset < 0)? '-' : '+'), h, m); + g_string_append_printf(buf, " %c%.2" PRIu32 ":%.2" PRIu32, + ((dt->offset < 0)? '-' : '+'), h, m); + } else { - offset += snprintf(result + offset, DATE_MAX - offset, "Z"); + g_string_append_c(buf, 'Z'); } } +done: crm_time_free(utc); + result = pcmk__str_copy(buf->str); + g_string_free(buf, TRUE); + return result; } /*! @@ -713,10 +719,7 @@ time_as_string_common(const crm_time_t *dt, int usec, uint32_t flags, char * crm_time_as_string(const crm_time_t *dt, int flags) { - char result[DATE_MAX] = { '\0', }; - - time_as_string_common(dt, 0, flags, result); - return pcmk__str_copy(result); + return time_as_string_common(dt, 0, flags); } /*! @@ -2184,15 +2187,13 @@ pcmk__timespec2str(const struct timespec *ts, uint32_t flags) { struct timespec tmp_ts; crm_time_t dt; - char result[DATE_MAX] = { 0 }; if (ts == NULL) { qb_util_timespec_from_epoch_get(&tmp_ts); ts = &tmp_ts; } crm_time_set_timet(&dt, &ts->tv_sec); - time_as_string_common(&dt, ts->tv_nsec / QB_TIME_NS_IN_USEC, flags, result); - return pcmk__str_copy(result); + return time_as_string_common(&dt, ts->tv_nsec / QB_TIME_NS_IN_USEC, flags); } /*! From 655b98e79c65d9a2378e0d130d6e05256d1670ff Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 19 Jun 2025 15:33:50 -0700 Subject: [PATCH 02/49] Refactor: libcrmcommon: Support usec in pcmk__time_format_hr() test Nothing uses this yet Signed-off-by: Reid Wahl --- .../tests/iso8601/pcmk__time_format_hr_test.c | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c index c4c9f7c65db..0dbcfef71dc 100644 --- a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c +++ b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c @@ -1,5 +1,5 @@ /* - * Copyright 2024 the Pacemaker project contributors + * Copyright 2024-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -24,17 +24,20 @@ * \param[in] expected Assertion succeeds if result matches this * \param[in] alternate If this is not NULL, assertion may also succeed if * result matches this + * \param[in] usec Microseconds component of the reference time * * \note This allows two possible results because different strftime() * implementations handle certain format syntax differently. */ static void assert_hr_format(const char *format, const char *expected, - const char *alternate) + const char *alternate, int usec) { pcmk__time_hr_t *hr = TEST_TIME; - char *result = pcmk__time_format_hr(format, hr); + char *result = NULL; + hr->useconds = usec; + result = pcmk__time_format_hr(format, hr); pcmk__time_hr_free(hr); if (expected == NULL) { @@ -58,55 +61,55 @@ static void null_format(void **state) { assert_null(pcmk__time_format_hr(NULL, NULL)); - assert_hr_format(NULL, NULL, NULL); // for pcmk__time_format_hr(NULL, hr) + assert_hr_format(NULL, NULL, NULL, 0); // for pcmk__time_format_hr(NULL, hr) } static void no_specifiers(void **state) { - assert_hr_format("no specifiers", "no specifiers", NULL); + assert_hr_format("no specifiers", "no specifiers", NULL, 0); assert_hr_format("this has a literal % in it", "this has a literal % in it", // *BSD strftime() strips single % - "this has a literal in it"); + "this has a literal in it", 0); assert_hr_format("this has a literal %01 in it", "this has a literal %01 in it", // *BSD strftime() strips %0 - "this has a literal 1 in it"); + "this has a literal 1 in it", 0); assert_hr_format("%2 this starts and ends with %", "%2 this starts and ends with %", // *BSD strftime() strips % in front of nonzero number - "2 this starts and ends with %"); + "2 this starts and ends with %", 0); /* strftime() treats % with a number (and no specifier) as a literal string * to be formatted with a field width (undocumented and probably a bug ...) */ assert_hr_format("this ends with %10", "this ends with %10", // *BSD strftime() strips % in front of nonzero number - "this ends with 10"); + "this ends with 10", 0); } static void without_nano(void **state) { - assert_hr_format("%H:%M %a %b %d", "03:04 Tue Jan 02", NULL); - assert_hr_format("%H:%M:%S", "03:04:05", NULL); + assert_hr_format("%H:%M %a %b %d", "03:04 Tue Jan 02", NULL, 0); + assert_hr_format("%H:%M:%S", "03:04:05", NULL, 0); assert_hr_format("The time is %H:%M right now", - "The time is 03:04 right now", NULL); + "The time is 03:04 right now", NULL, 0); assert_hr_format("%3S seconds", "005 seconds", // *BSD strftime() doesn't support field widths - "3S seconds"); + "3S seconds", 0); // strftime() treats %% as a literal % - assert_hr_format("%%H %%N", "%H %N", NULL); + assert_hr_format("%%H %%N", "%H %N", NULL, 0); } static void with_nano(void **state) { - assert_hr_format("%H:%M:%S.%06N", "03:04:05.000000", NULL); + assert_hr_format("%H:%M:%S.%06N", "03:04:05.000000", NULL, 0); assert_hr_format("The time is %H:%M:%S.%06N right NOW", - "The time is 03:04:05.000000 right NOW", NULL); + "The time is 03:04:05.000000 right NOW", NULL, 0); } PCMK__UNIT_TEST(NULL, NULL, From 960d0422419fffb0f7b6571a7209d6a6166b7ecb Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 19 Jun 2025 15:49:14 -0700 Subject: [PATCH 03/49] Test: libcrmcommon: Test pcmk__time_format_hr() with nonzero usec The %N formatting specifier behavior is not obvious, but we're stuck with it now. Future commits will at least clarify it. Signed-off-by: Reid Wahl --- .../tests/iso8601/pcmk__time_format_hr_test.c | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c index 0dbcfef71dc..41cee602ea0 100644 --- a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c +++ b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c @@ -107,9 +107,34 @@ without_nano(void **state) static void with_nano(void **state) { + int usec = 123456; + + // Display time with no fractional seconds component assert_hr_format("%H:%M:%S.%06N", "03:04:05.000000", NULL, 0); + assert_hr_format("%H:%M:%S.%03N", "03:04:05.000", NULL, 0); + assert_hr_format("%H:%M:%S.%00N", "03:04:05.", NULL, 0); + assert_hr_format("%H:%M:%S.%0N", "03:04:05.", NULL, 0); assert_hr_format("The time is %H:%M:%S.%06N right NOW", "The time is 03:04:05.000000 right NOW", NULL, 0); + + // Display at appropriate resolution by truncating toward zero + assert_hr_format("%H:%M:%S.%06N", "03:04:05.123456", NULL, usec); + assert_hr_format("%H:%M:%S.%05N", "03:04:05.12345", NULL, usec); + assert_hr_format("%H:%M:%S.%04N", "03:04:05.1234", NULL, usec); + assert_hr_format("%H:%M:%S.%03N", "03:04:05.123", NULL, usec); + assert_hr_format("%H:%M:%S.%02N", "03:04:05.12", NULL, usec); + assert_hr_format("%H:%M:%S.%01N", "03:04:05.1", NULL, usec); + assert_hr_format("%H:%M:%S.%00N", "03:04:05.", NULL, usec); + + // Same as above, but with correct zero-padding + usec = 789; + assert_hr_format("%H:%M:%S.%06N", "03:04:05.000789", NULL, usec); + assert_hr_format("%H:%M:%S.%05N", "03:04:05.00078", NULL, usec); + assert_hr_format("%H:%M:%S.%04N", "03:04:05.0007", NULL, usec); + assert_hr_format("%H:%M:%S.%03N", "03:04:05.000", NULL, usec); + assert_hr_format("%H:%M:%S.%02N", "03:04:05.00", NULL, usec); + assert_hr_format("%H:%M:%S.%01N", "03:04:05.0", NULL, usec); + assert_hr_format("%H:%M:%S.%00N", "03:04:05.", NULL, usec); } PCMK__UNIT_TEST(NULL, NULL, From 0fcc669cd3b7fed3f9896ffb380888df57c0b621 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Tue, 17 Jun 2025 01:03:37 -0700 Subject: [PATCH 04/49] Refactor: libcrmcommon: Avoid snprintf() in pcmk__time_format_hr() We weren't error-checking it, and this way we don't have to. GString functions assert on memory error. Ref T974 Signed-off-by: Reid Wahl --- lib/common/iso8601.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/lib/common/iso8601.c b/lib/common/iso8601.c index ec7ccb324ed..c59357038de 100644 --- a/lib/common/iso8601.c +++ b/lib/common/iso8601.c @@ -2033,10 +2033,9 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) { int scanned_pos = 0; // How many characters of format have been parsed int printed_pos = 0; // How many characters of format have been processed - size_t date_len = 0; - char nano_s[10] = { '\0', }; - char date_s[128] = { '\0', }; + GString *buf = NULL; + char *result = NULL; struct tm tm = { 0, }; crm_time_t dt = { 0, }; @@ -2044,6 +2043,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) if (format == NULL) { return NULL; } + buf = g_string_sized_new(128); pcmk__time_set_hr_dt(&dt, hr_dt); ha_get_tm_time(&tm, &dt); sprintf(nano_s, "%06d000", hr_dt->useconds); @@ -2052,6 +2052,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) int fmt_pos; // Index after last character to pass as-is int nano_digits = 0; // Length of %N field width (if any) char *tmp_fmt_s = NULL; + char date_s[128] = { '\0', }; size_t nbytes = 0; // Look for next format specifier @@ -2094,10 +2095,6 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) } } - if (date_len >= sizeof(date_s)) { - return NULL; // No room for remaining string - } - tmp_fmt_s = strndup(&format[printed_pos], fmt_pos - printed_pos); if (tmp_fmt_s == NULL) { return NULL; @@ -2107,34 +2104,29 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wformat-nonliteral" #endif - nbytes = strftime(&date_s[date_len], sizeof(date_s) - date_len, - tmp_fmt_s, &tm); + nbytes = strftime(date_s, sizeof(date_s), tmp_fmt_s, &tm); #ifdef HAVE_FORMAT_NONLITERAL #pragma GCC diagnostic pop #endif free(tmp_fmt_s); if (nbytes == 0) { // Would overflow buffer - return NULL; + g_string_truncate(buf, 0); + goto done; } - date_len += nbytes; + + g_string_append(buf, date_s); printed_pos = scanned_pos; if (nano_digits != 0) { - int nc = 0; - - if (date_len >= sizeof(date_s)) { - return NULL; // No room to add nanoseconds - } - nc = snprintf(&date_s[date_len], sizeof(date_s) - date_len, - "%.*s", nano_digits, nano_s); - - if ((nc < 0) || (nc == (sizeof(date_s) - date_len))) { - return NULL; // Error or would overflow buffer - } - date_len += nc; + g_string_append_printf(buf, "%.*s", nano_digits, nano_s); } } - return (date_len == 0)? NULL : pcmk__str_copy(date_s); +done: + if (buf->len > 0) { + result = pcmk__str_copy(buf->str); + } + g_string_free(buf, TRUE); + return result; } /*! From 8d47cea0b1b47f8addbb8f2c432fb08b1dd37b9d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 19 Jun 2025 00:55:07 -0700 Subject: [PATCH 05/49] Refactor: libcrmcommon: Clarify fractional seconds in time format The use of "%N" and "nano_*" were misleading. We only support storing and displaying times at microsecond resolution or less. * Storing: Note that we fetch hr_dt->useconds and append three zeros unconditionally in the string representation (as nanoseconds). * Displaying: Note the `nano_digits = QB_MIN(nano_digits, 6)`. This caps the displayed field width so that the fractional (decimal) component of a timestamp is at microsecond resolution or coarser. So we can never display any of the three zeros that were appended above. We have to keep "%N" and the existing behavior for backward compatibility, of course. Signed-off-by: Reid Wahl --- lib/common/iso8601.c | 38 ++++++++++++------- .../tests/iso8601/pcmk__time_format_hr_test.c | 8 ++-- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/lib/common/iso8601.c b/lib/common/iso8601.c index c59357038de..d5994aab357 100644 --- a/lib/common/iso8601.c +++ b/lib/common/iso8601.c @@ -2020,10 +2020,10 @@ pcmk__time_hr_free(pcmk__time_hr_t * hr_dt) /*! * \internal - * \brief Expand a date/time format string, including %N for nanoseconds + * \brief Expand a date/time format string, including %N for fractional seconds * - * \param[in] format Date/time format string as per strftime(3) with the - * addition of %N for nanoseconds + * \param[in] format Date/time format string as per \c strftime(3) with the + * addition of %N for fractional seconds * \param[in] hr_dt Time value to format * * \return Newly allocated string with formatted string @@ -2033,7 +2033,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) { int scanned_pos = 0; // How many characters of format have been parsed int printed_pos = 0; // How many characters of format have been processed - char nano_s[10] = { '\0', }; + char frac_s[10] = { '\0', }; GString *buf = NULL; char *result = NULL; @@ -2046,11 +2046,11 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) buf = g_string_sized_new(128); pcmk__time_set_hr_dt(&dt, hr_dt); ha_get_tm_time(&tm, &dt); - sprintf(nano_s, "%06d000", hr_dt->useconds); + sprintf(frac_s, "%06d", hr_dt->useconds); while (format[scanned_pos] != '\0') { - int fmt_pos; // Index after last character to pass as-is - int nano_digits = 0; // Length of %N field width (if any) + int fmt_pos = 0; // Index after last character to pass as-is + int frac_digits = 0; // %N specifier's width field value (if any) char *tmp_fmt_s = NULL; char date_s[128] = { '\0', }; size_t nbytes = 0; @@ -2066,7 +2066,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) } else { fmt_pos = mark_s - format; // Index of % - // Skip % and any field width + // Skip % and any width field scanned_pos = fmt_pos + 1; while (isdigit(format[scanned_pos])) { scanned_pos++; @@ -2078,12 +2078,22 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) break; case 'N': // %[width]N + /* Fractional seconds. This was supposed to represent + * nanoseconds. However, we only store times at microsecond + * resolution, and the width field support makes this a + * general fractional component specifier rather than a + * nanoseconds specifier. + * + * Further, since we cap the width at 6 digits, a user + * cannot display times at greater than microsecond + * resolution. + */ scanned_pos++; - // Parse field width - nano_digits = atoi(&format[fmt_pos + 1]); - nano_digits = QB_MAX(nano_digits, 0); - nano_digits = QB_MIN(nano_digits, 6); + // Parse width field + frac_digits = atoi(&format[fmt_pos + 1]); + frac_digits = QB_MAX(frac_digits, 0); + frac_digits = QB_MIN(frac_digits, 6); break; default: // Some other specifier @@ -2116,8 +2126,8 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) g_string_append(buf, date_s); printed_pos = scanned_pos; - if (nano_digits != 0) { - g_string_append_printf(buf, "%.*s", nano_digits, nano_s); + if (frac_digits != 0) { + g_string_append_printf(buf, "%.*s", frac_digits, frac_s); } } diff --git a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c index 41cee602ea0..1915a3033cf 100644 --- a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c +++ b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c @@ -90,7 +90,7 @@ no_specifiers(void **state) } static void -without_nano(void **state) +without_frac(void **state) { assert_hr_format("%H:%M %a %b %d", "03:04 Tue Jan 02", NULL, 0); assert_hr_format("%H:%M:%S", "03:04:05", NULL, 0); @@ -105,7 +105,7 @@ without_nano(void **state) } static void -with_nano(void **state) +with_frac(void **state) { int usec = 123456; @@ -140,5 +140,5 @@ with_nano(void **state) PCMK__UNIT_TEST(NULL, NULL, cmocka_unit_test(null_format), cmocka_unit_test(no_specifiers), - cmocka_unit_test(without_nano), - cmocka_unit_test(with_nano)) + cmocka_unit_test(without_frac), + cmocka_unit_test(with_frac)) From 335a2acb7ce31430b68d8b9d06da2644a8bd5172 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 19 Jun 2025 02:05:37 -0700 Subject: [PATCH 06/49] Refactor: libcrmcommon: Avoid sprintf() in pcmk__time_format_hr() We weren't error-checking sprintf() before. Just use g_string_append_printf() so that the error checking is done for us. Ref T974 Signed-off-by: Reid Wahl --- lib/common/iso8601.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/common/iso8601.c b/lib/common/iso8601.c index d5994aab357..950ac88f221 100644 --- a/lib/common/iso8601.c +++ b/lib/common/iso8601.c @@ -2033,7 +2033,6 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) { int scanned_pos = 0; // How many characters of format have been parsed int printed_pos = 0; // How many characters of format have been processed - char frac_s[10] = { '\0', }; GString *buf = NULL; char *result = NULL; @@ -2046,7 +2045,6 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) buf = g_string_sized_new(128); pcmk__time_set_hr_dt(&dt, hr_dt); ha_get_tm_time(&tm, &dt); - sprintf(frac_s, "%06d", hr_dt->useconds); while (format[scanned_pos] != '\0') { int fmt_pos = 0; // Index after last character to pass as-is @@ -2127,7 +2125,24 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) g_string_append(buf, date_s); printed_pos = scanned_pos; if (frac_digits != 0) { - g_string_append_printf(buf, "%.*s", frac_digits, frac_s); + // Descending powers of 10 (10^5 down to 10^0) + static const int powers[6] = { 1e5, 1e4, 1e3, 1e2, 1e1, 1e0 }; + + // Sanity check to ensure array access is in bounds + pcmk__assert((frac_digits > 0) && (frac_digits <= 6)); + + /* Append fractional seconds at the requested resolution, truncated + * toward zero. We're basically converting from microseconds to + * another unit here. For example, suppose the width field + * (frac_digits) is 3. This means "use millisecond resolution." Then + * we need to divide our microseconds value by 10^3, which is + * powers[3 - 1]. + * + * If the width field is 6 (microsecond resolution), then we divide + * our microseconds value by 10^0 == 1, which is powers[6 - 1]. + */ + g_string_append_printf(buf, "%0*d", frac_digits, + hr_dt->useconds / powers[frac_digits - 1]); } } From 4b075d21efb006ff87fc0afd95ca330d879f7b77 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 19 Jun 2025 02:13:36 -0700 Subject: [PATCH 07/49] Refactor: libcrmcommon: Use g_strndup() in pcmk__time_format_hr() This asserts on error. Signed-off-by: Reid Wahl --- lib/common/iso8601.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/common/iso8601.c b/lib/common/iso8601.c index 950ac88f221..2c3ffffe459 100644 --- a/lib/common/iso8601.c +++ b/lib/common/iso8601.c @@ -2049,7 +2049,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) while (format[scanned_pos] != '\0') { int fmt_pos = 0; // Index after last character to pass as-is int frac_digits = 0; // %N specifier's width field value (if any) - char *tmp_fmt_s = NULL; + gchar *tmp_fmt_s = NULL; char date_s[128] = { '\0', }; size_t nbytes = 0; @@ -2103,10 +2103,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) } } - tmp_fmt_s = strndup(&format[printed_pos], fmt_pos - printed_pos); - if (tmp_fmt_s == NULL) { - return NULL; - } + tmp_fmt_s = g_strndup(&format[printed_pos], fmt_pos - printed_pos); #ifdef HAVE_FORMAT_NONLITERAL #pragma GCC diagnostic push @@ -2116,7 +2113,7 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) #ifdef HAVE_FORMAT_NONLITERAL #pragma GCC diagnostic pop #endif - free(tmp_fmt_s); + g_free(tmp_fmt_s); if (nbytes == 0) { // Would overflow buffer g_string_truncate(buf, 0); goto done; From 4d739a9ba7e2afb4d04ec5358d9fb4b20166106a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 19 Jun 2025 02:08:18 -0700 Subject: [PATCH 08/49] Doc: Pacemaker Explained: Clarify alert timestamp-format A leading zero is ignored for "%xN", so remove it from the example. Add detail to the description. Also, we strftime() into a fixed-size buffer of 128 characters. For some reason, there is no general way to strftime into a dynamically sized buffer or to determine how large the buffer needs to be. Signed-off-by: Reid Wahl --- doc/sphinx/Pacemaker_Explained/alerts.rst | 17 +++++++- lib/common/iso8601.c | 4 ++ .../tests/iso8601/pcmk__time_format_hr_test.c | 39 +++++++++++-------- 3 files changed, 41 insertions(+), 19 deletions(-) diff --git a/doc/sphinx/Pacemaker_Explained/alerts.rst b/doc/sphinx/Pacemaker_Explained/alerts.rst index 27000ed9410..b573bcf2d4f 100644 --- a/doc/sphinx/Pacemaker_Explained/alerts.rst +++ b/doc/sphinx/Pacemaker_Explained/alerts.rst @@ -114,14 +114,27 @@ whether and how Pacemaker calls them. | | | recipient of that alert, that recipient will not be | | | | used. *(since 2.1.6)* | +------------------+---------------+-----------------------------------------------------+ - | timestamp-format | %H:%M:%S.%06N | .. index:: | + | timestamp-format | %H:%M:%S.%6N | .. index:: | | | | single: alert; meta-attribute, timestamp-format | | | | single: meta-attribute; timestamp-format (alert) | | | | single: timestamp-format; alert meta-attribute | | | | | | | | Format the cluster will use when sending the | | | | event's timestamp to the agent. This is a string as | - | | | used with the ``date(1)`` command. | + | | | used with the ``date(1)`` command, with the | + | | | following extension. ``"%xN"``, where ``x`` is a | + | | | number with ``1 <= x <= 6``, prints the fractional | + | | | seconds component of the timestamp at ``10^(-x)`` | + | | | resolution, without a decimal point (``'.'``). | + | | | Values are truncated toward zero, not rounded. | + | | | | + | | | Note: This is implemented using ``strftime()`` with | + | | | a 128-character buffer. If any format specifier's | + | | | expansion requires more than 128 characters, or if | + | | | any specifier expands to an empty string, then the | + | | | timestamp is discarded. (Expanding to an empty | + | | | string is not an error, but there is no way to | + | | | distinguish this from a too-small buffer.) | +------------------+---------------+-----------------------------------------------------+ | timeout | 30s | .. index:: | | | | single: alert; meta-attribute, timeout | diff --git a/lib/common/iso8601.c b/lib/common/iso8601.c index 2c3ffffe459..bac00524d41 100644 --- a/lib/common/iso8601.c +++ b/lib/common/iso8601.c @@ -2085,6 +2085,10 @@ pcmk__time_format_hr(const char *format, const pcmk__time_hr_t *hr_dt) * Further, since we cap the width at 6 digits, a user * cannot display times at greater than microsecond * resolution. + * + * A leading zero in the width field is ignored, not treated + * as "use zero-padding." For example, "%03N" and "%3N" + * produce the same result. */ scanned_pos++; diff --git a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c index 1915a3033cf..c41ff0bfc69 100644 --- a/lib/common/tests/iso8601/pcmk__time_format_hr_test.c +++ b/lib/common/tests/iso8601/pcmk__time_format_hr_test.c @@ -110,31 +110,36 @@ with_frac(void **state) int usec = 123456; // Display time with no fractional seconds component - assert_hr_format("%H:%M:%S.%06N", "03:04:05.000000", NULL, 0); - assert_hr_format("%H:%M:%S.%03N", "03:04:05.000", NULL, 0); - assert_hr_format("%H:%M:%S.%00N", "03:04:05.", NULL, 0); + assert_hr_format("%H:%M:%S.%6N", "03:04:05.000000", NULL, 0); + assert_hr_format("%H:%M:%S.%3N", "03:04:05.000", NULL, 0); assert_hr_format("%H:%M:%S.%0N", "03:04:05.", NULL, 0); + assert_hr_format("%H:%M:%S.%N", "03:04:05.", NULL, 0); assert_hr_format("The time is %H:%M:%S.%06N right NOW", "The time is 03:04:05.000000 right NOW", NULL, 0); // Display at appropriate resolution by truncating toward zero - assert_hr_format("%H:%M:%S.%06N", "03:04:05.123456", NULL, usec); - assert_hr_format("%H:%M:%S.%05N", "03:04:05.12345", NULL, usec); - assert_hr_format("%H:%M:%S.%04N", "03:04:05.1234", NULL, usec); + assert_hr_format("%H:%M:%S.%6N", "03:04:05.123456", NULL, usec); + assert_hr_format("%H:%M:%S.%5N", "03:04:05.12345", NULL, usec); + assert_hr_format("%H:%M:%S.%4N", "03:04:05.1234", NULL, usec); + assert_hr_format("%H:%M:%S.%3N", "03:04:05.123", NULL, usec); + assert_hr_format("%H:%M:%S.%2N", "03:04:05.12", NULL, usec); + assert_hr_format("%H:%M:%S.%1N", "03:04:05.1", NULL, usec); + assert_hr_format("%H:%M:%S.%0N", "03:04:05.", NULL, usec); + assert_hr_format("%H:%M:%S.%N", "03:04:05.", NULL, usec); + + // Leading zero is ignored, not treated as a request for zero-padding assert_hr_format("%H:%M:%S.%03N", "03:04:05.123", NULL, usec); - assert_hr_format("%H:%M:%S.%02N", "03:04:05.12", NULL, usec); - assert_hr_format("%H:%M:%S.%01N", "03:04:05.1", NULL, usec); - assert_hr_format("%H:%M:%S.%00N", "03:04:05.", NULL, usec); - // Same as above, but with correct zero-padding + // Same as above, but with zero-padding for smaller fractional components usec = 789; - assert_hr_format("%H:%M:%S.%06N", "03:04:05.000789", NULL, usec); - assert_hr_format("%H:%M:%S.%05N", "03:04:05.00078", NULL, usec); - assert_hr_format("%H:%M:%S.%04N", "03:04:05.0007", NULL, usec); - assert_hr_format("%H:%M:%S.%03N", "03:04:05.000", NULL, usec); - assert_hr_format("%H:%M:%S.%02N", "03:04:05.00", NULL, usec); - assert_hr_format("%H:%M:%S.%01N", "03:04:05.0", NULL, usec); - assert_hr_format("%H:%M:%S.%00N", "03:04:05.", NULL, usec); + assert_hr_format("%H:%M:%S.%6N", "03:04:05.000789", NULL, usec); + assert_hr_format("%H:%M:%S.%5N", "03:04:05.00078", NULL, usec); + assert_hr_format("%H:%M:%S.%4N", "03:04:05.0007", NULL, usec); + assert_hr_format("%H:%M:%S.%3N", "03:04:05.000", NULL, usec); + assert_hr_format("%H:%M:%S.%2N", "03:04:05.00", NULL, usec); + assert_hr_format("%H:%M:%S.%1N", "03:04:05.0", NULL, usec); + assert_hr_format("%H:%M:%S.%0N", "03:04:05.", NULL, usec); + assert_hr_format("%H:%M:%S.%N", "03:04:05.", NULL, usec); } PCMK__UNIT_TEST(NULL, NULL, From 6bad1820be24b62708b125994f70174a1761a3a8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 00:03:59 -0700 Subject: [PATCH 09/49] Refactor: based: Error-check snprintf() in cib_digester_cb() Ref T974 Signed-off-by: Reid Wahl --- daemons/based/based_callbacks.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/daemons/based/based_callbacks.c b/daemons/based/based_callbacks.c index 13646eb4be0..1d34055d9d3 100644 --- a/daemons/based/based_callbacks.c +++ b/daemons/based/based_callbacks.c @@ -414,7 +414,8 @@ cib_digester_cb(gpointer data) free(ping_digest); ping_digest = NULL; ping_modified_since = FALSE; - snprintf(buffer, 32, "%" PRIu64, ping_seq); + pcmk__assert(snprintf(buffer, 32, "%" PRIu64, ping_seq) >= 0); + crm_trace("Requesting peer digests (%s)", buffer); crm_xml_add(ping, PCMK__XA_T, PCMK__VALUE_CIB); From b8316fab48a0bca4fde42919869dcaba16b4f889 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 00:14:35 -0700 Subject: [PATCH 10/49] Refactor: executor: Error-check snprintf() in read_events() Ref T974 Signed-off-by: Reid Wahl --- daemons/execd/cts-exec-helper.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/daemons/execd/cts-exec-helper.c b/daemons/execd/cts-exec-helper.c index 47ab5bbe382..9a032871365 100644 --- a/daemons/execd/cts-exec-helper.c +++ b/daemons/execd/cts-exec-helper.c @@ -1,5 +1,5 @@ /* - * Copyright 2012-2024 the Pacemaker project contributors + * Copyright 2012-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -160,8 +160,6 @@ static GOptionEntry api_call_entries[] = { static GMainLoop *mainloop = NULL; static lrmd_t *lrmd_conn = NULL; -static char event_buf_v0[1024]; - static crm_exit_t test_exit(crm_exit_t exit_code) { @@ -174,15 +172,6 @@ test_exit(crm_exit_t exit_code) printf(fmt "\n" , ##args); \ } -#define report_event(event) \ - snprintf(event_buf_v0, sizeof(event_buf_v0), "NEW_EVENT event_type:%s rsc_id:%s action:%s rc:%s op_status:%s", \ - lrmd_event_type2str(event->type), \ - event->rsc_id, \ - event->op_type ? event->op_type : "none", \ - crm_exit_str((crm_exit_t) event->rc), \ - pcmk_exec_status_str(event->op_status)); \ - crm_info("%s", event_buf_v0); - static void test_shutdown(int nsig) { @@ -193,12 +182,20 @@ test_shutdown(int nsig) static void read_events(lrmd_event_data_t * event) { - report_event(event); - if (options.listen) { - if (pcmk__str_eq(options.listen, event_buf_v0, pcmk__str_casei)) { - print_result("LISTEN EVENT SUCCESSFUL"); - test_exit(CRM_EX_OK); - } + char buf[1024] = { '\0', }; + + pcmk__assert(snprintf(buf, sizeof(buf), + "NEW_EVENT event_type:%s rsc_id:%s action:%s rc:%s " + "op_status:%s", + lrmd_event_type2str(event->type), event->rsc_id, + pcmk__s(event->op_type, "none"), + crm_exit_str((crm_exit_t) event->rc), + pcmk_exec_status_str(event->op_status)) >= 0); + crm_info("%s", buf); + + if (options.listen && pcmk__str_eq(options.listen, buf, pcmk__str_casei)) { + print_result("LISTEN EVENT SUCCESSFUL"); + test_exit(CRM_EX_OK); } if (exec_call_id && (event->call_id == exec_call_id)) { From d001021c98f7bb3268c4489ee93c0b9ee2d6bbae Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 01:33:35 -0700 Subject: [PATCH 11/49] Refactor: executor: Avoid snprintf() in get_address_info() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Right now we can be sure port is at most 65536, but it's simpler not to use that assumption in get_address_info() anyway. This extra allocation shouldn't impact performance in any significant way. Ref T974 Signed-off-by: Reid Wahl --- daemons/execd/remoted_tls.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/daemons/execd/remoted_tls.c b/daemons/execd/remoted_tls.c index eede13edaec..13c867f3e63 100644 --- a/daemons/execd/remoted_tls.c +++ b/daemons/execd/remoted_tls.c @@ -319,8 +319,8 @@ bind_and_listen(struct addrinfo *addr) static int get_address_info(const char *bind_name, int port, struct addrinfo **res) { - int rc; - char port_str[6]; // at most "65535" + int rc = pcmk_rc_ok; + char *port_s = pcmk__itoa(port); struct addrinfo hints; memset(&hints, 0, sizeof(struct addrinfo)); @@ -329,17 +329,16 @@ get_address_info(const char *bind_name, int port, struct addrinfo **res) hints.ai_socktype = SOCK_STREAM; hints.ai_protocol = IPPROTO_TCP; - snprintf(port_str, sizeof(port_str), "%d", port); - rc = getaddrinfo(bind_name, port_str, &hints, res); + rc = getaddrinfo(bind_name, port_s, &hints, res); rc = pcmk__gaierror2rc(rc); if (rc != pcmk_rc_ok) { crm_err("Unable to get IP address(es) for %s: %s", (bind_name? bind_name : "local node"), pcmk_rc_str(rc)); - return rc; } - return pcmk_rc_ok; + free(port_s); + return rc; } int From 8ed2743b0e929d2cb83b63c93b83d71a704f5c0c Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 01:37:40 -0700 Subject: [PATCH 12/49] Refactor: fencer: Avoid snprintf() in test_register_async_devices() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- daemons/fenced/cts-fence-helper.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/daemons/fenced/cts-fence-helper.c b/daemons/fenced/cts-fence-helper.c index cc83fd32a31..42c1730324e 100644 --- a/daemons/fenced/cts-fence-helper.c +++ b/daemons/fenced/cts-fence-helper.c @@ -497,7 +497,8 @@ test_async_monitor(int check_event) static void test_register_async_devices(int check_event) { - char buf[16] = { 0, }; + char *off_timeout_s = pcmk__itoa(MAINLOOP_DEFAULT_TIMEOUT + + CUSTOM_TIMEOUT_ADDITION); stonith_key_value_t *params = NULL; params = stonith__key_value_add(params, PCMK_STONITH_HOST_MAP, @@ -518,12 +519,12 @@ test_register_async_devices(int check_event) "custom_timeout_node1=1,2"); params = stonith__key_value_add(params, "mode", "fail"); params = stonith__key_value_add(params, "delay", "1000"); - snprintf(buf, sizeof(buf) - 1, "%d", MAINLOOP_DEFAULT_TIMEOUT + CUSTOM_TIMEOUT_ADDITION); - params = stonith__key_value_add(params, "pcmk_off_timeout", buf); + params = stonith__key_value_add(params, "pcmk_off_timeout", off_timeout_s); st->cmds->register_device(st, st_opts, "false_custom_timeout", "stonith-ng", "fence_dummy", params); stonith__key_value_freeall(params, true, true); + free(off_timeout_s); mainloop_test_done(__func__, true); } From cdeccf65500f121334b32c69babe5db28fbf9fec Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:02:59 -0700 Subject: [PATCH 13/49] Refactor: fencer: Avoid snprintf() in get_action_timeout() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 68c9a6df592..9934760ea5e 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -272,7 +272,7 @@ get_action_timeout(const fenced_device_t *device, const char *action, int default_timeout) { if (action && device && device->params) { - char buffer[64] = { 0, }; + char *timeout_param = NULL; const char *value = NULL; /* If "reboot" was requested but the device does not support it, @@ -286,8 +286,10 @@ get_action_timeout(const fenced_device_t *device, const char *action, } /* If the device config specified an action-specific timeout, use it */ - snprintf(buffer, sizeof(buffer), "pcmk_%s_timeout", action); - value = g_hash_table_lookup(device->params, buffer); + timeout_param = crm_strdup_printf("pcmk_%s_timeout", action); + value = g_hash_table_lookup(device->params, timeout_param); + free(timeout_param); + if (value) { long long timeout_ms = crm_get_msec(value); return (int) QB_MIN(pcmk__timeout_ms2s(timeout_ms), INT_MAX); From e63bdba94c4e8e1bb2cf3c34bd60b590d856e3df Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:06:11 -0700 Subject: [PATCH 14/49] Refactor: pacemakerd: Avoid snprintf() in pacemakerd_read_config() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- daemons/pacemakerd/pcmkd_corosync.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/daemons/pacemakerd/pcmkd_corosync.c b/daemons/pacemakerd/pcmkd_corosync.c index 7e54c42c4a8..f0e68d749f4 100644 --- a/daemons/pacemakerd/pcmkd_corosync.c +++ b/daemons/pacemakerd/pcmkd_corosync.c @@ -1,5 +1,5 @@ /* - * Copyright 2010-2024 the Pacemaker project contributors + * Copyright 2010-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -360,9 +360,11 @@ pacemakerd_read_config(void) " No group found for user %s", CRM_DAEMON_USER); } else { - char key[PATH_MAX]; - snprintf(key, PATH_MAX, "uidgid.gid.%u", gid); + char *key = crm_strdup_printf("uidgid.gid.%lld", (long long) gid); + rc = cmap_set_uint8(local_handle, key, 1); + free(key); + if (rc != CS_OK) { crm_warn("Could not authorize group with Corosync: %s " QB_XS " group=%u rc=%d", pcmk__cs_err_str(rc), gid, rc); From 4c1024fadca2c27d9a9933cac321407c07269678 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:11:49 -0700 Subject: [PATCH 15/49] Refactor: libcrmcommon: Avoid snprintf() in no_pids()/has_pids() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- .../tests/procfs/pcmk__procfs_has_pids_false_test.c | 10 ++++------ .../tests/procfs/pcmk__procfs_has_pids_true_test.c | 9 ++++----- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/lib/common/tests/procfs/pcmk__procfs_has_pids_false_test.c b/lib/common/tests/procfs/pcmk__procfs_has_pids_false_test.c index 4601aac9df6..9fc73bc5df7 100644 --- a/lib/common/tests/procfs/pcmk__procfs_has_pids_false_test.c +++ b/lib/common/tests/procfs/pcmk__procfs_has_pids_false_test.c @@ -1,5 +1,5 @@ /* - * Copyright 2022 the Pacemaker project contributors + * Copyright 2022-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -17,18 +17,15 @@ #include #include - static void no_pids(void **state) { - char path[PATH_MAX]; - - snprintf(path, PATH_MAX, "/proc/%u/exe", getpid()); + char *exe_path = crm_strdup_printf("/proc/%lld/exe", (long long) getpid()); // Set readlink() errno and link contents (for /proc/PID/exe) pcmk__mock_readlink = true; - expect_string(__wrap_readlink, path, path); + expect_string(__wrap_readlink, path, exe_path); expect_any(__wrap_readlink, buf); expect_value(__wrap_readlink, bufsize, PATH_MAX - 1); will_return(__wrap_readlink, ENOENT); @@ -37,6 +34,7 @@ no_pids(void **state) assert_false(pcmk__procfs_has_pids()); pcmk__mock_readlink = false; + free(exe_path); } PCMK__UNIT_TEST(NULL, NULL, cmocka_unit_test(no_pids)) diff --git a/lib/common/tests/procfs/pcmk__procfs_has_pids_true_test.c b/lib/common/tests/procfs/pcmk__procfs_has_pids_true_test.c index 758e3b905c5..65a4a320bd4 100644 --- a/lib/common/tests/procfs/pcmk__procfs_has_pids_true_test.c +++ b/lib/common/tests/procfs/pcmk__procfs_has_pids_true_test.c @@ -1,5 +1,5 @@ /* - * Copyright 2022 the Pacemaker project contributors + * Copyright 2022-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -20,14 +20,12 @@ static void has_pids(void **state) { - char path[PATH_MAX]; - - snprintf(path, PATH_MAX, "/proc/%u/exe", getpid()); + char *exe_path = crm_strdup_printf("/proc/%lld/exe", (long long) getpid()); // Set readlink() errno and link contents (for /proc/PID/exe) pcmk__mock_readlink = true; - expect_string(__wrap_readlink, path, path); + expect_string(__wrap_readlink, path, exe_path); expect_any(__wrap_readlink, buf); expect_value(__wrap_readlink, bufsize, PATH_MAX - 1); will_return(__wrap_readlink, 0); @@ -36,6 +34,7 @@ has_pids(void **state) assert_true(pcmk__procfs_has_pids()); pcmk__mock_readlink = false; + free(exe_path); } PCMK__UNIT_TEST(NULL, NULL, cmocka_unit_test(has_pids)) From 107a7799ab28472fb16a3153ba1005e1879f19d1 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:18:24 -0700 Subject: [PATCH 16/49] Refactor: libcrmcommon: Avoid snprintf() in pcmk__scan_double() tests To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- .../tests/strings/pcmk__scan_double_test.c | 45 ++++++++++--------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/lib/common/tests/strings/pcmk__scan_double_test.c b/lib/common/tests/strings/pcmk__scan_double_test.c index a1a180a215e..8eb43e46368 100644 --- a/lib/common/tests/strings/pcmk__scan_double_test.c +++ b/lib/common/tests/strings/pcmk__scan_double_test.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2022 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -14,9 +14,6 @@ #include // DBL_MAX, etc. #include // fabs() -// Ensure plenty of characters for %f display -#define LOCAL_BUF_SIZE 2 * DBL_MAX_10_EXP - /* * assert_float_equal doesn't exist for older versions of cmocka installed on some * of our builders, so define it in terms of regular assert() here in that case. @@ -85,8 +82,8 @@ no_result_variable(void **state) static void typical_case(void **state) { - char str[LOCAL_BUF_SIZE]; - double result; + char *buf = NULL; + double result = 0; assert_int_equal(pcmk__scan_double("0.0", &result, NULL, NULL), pcmk_rc_ok); assert_float_equal(result, 0.0, DBL_EPSILON); @@ -97,38 +94,42 @@ typical_case(void **state) assert_int_equal(pcmk__scan_double("-1.0", &result, NULL, NULL), pcmk_rc_ok); assert_float_equal(result, -1.0, DBL_EPSILON); - snprintf(str, LOCAL_BUF_SIZE, "%f", DBL_MAX); - assert_int_equal(pcmk__scan_double(str, &result, NULL, NULL), pcmk_rc_ok); + buf = crm_strdup_printf("%f", DBL_MAX); + assert_int_equal(pcmk__scan_double(buf, &result, NULL, NULL), pcmk_rc_ok); assert_float_equal(result, DBL_MAX, DBL_EPSILON); + free(buf); - snprintf(str, LOCAL_BUF_SIZE, "%f", -DBL_MAX); - assert_int_equal(pcmk__scan_double(str, &result, NULL, NULL), pcmk_rc_ok); + buf = crm_strdup_printf("%f", -DBL_MAX); + assert_int_equal(pcmk__scan_double(buf, &result, NULL, NULL), pcmk_rc_ok); assert_float_equal(result, -DBL_MAX, DBL_EPSILON); + free(buf); } static void double_overflow(void **state) { - char str[LOCAL_BUF_SIZE]; + char *buf = NULL; double result; /* * 1e(DBL_MAX_10_EXP + 1) produces an inf value * Can't use assert_float_equal() because (inf - inf) == NaN */ - snprintf(str, LOCAL_BUF_SIZE, "1e%d", DBL_MAX_10_EXP + 1); - assert_int_equal(pcmk__scan_double(str, &result, NULL, NULL), EOVERFLOW); + buf = crm_strdup_printf("1e%d", DBL_MAX_10_EXP + 1); + assert_int_equal(pcmk__scan_double(buf, &result, NULL, NULL), EOVERFLOW); assert_true(result > DBL_MAX); + free(buf); - snprintf(str, LOCAL_BUF_SIZE, "-1e%d", DBL_MAX_10_EXP + 1); - assert_int_equal(pcmk__scan_double(str, &result, NULL, NULL), EOVERFLOW); + buf = crm_strdup_printf("-1e%d", DBL_MAX_10_EXP + 1); + assert_int_equal(pcmk__scan_double(buf, &result, NULL, NULL), EOVERFLOW); assert_true(result < -DBL_MAX); + free(buf); } static void double_underflow(void **state) { - char str[LOCAL_BUF_SIZE]; + char *buf = NULL; double result; /* @@ -137,15 +138,19 @@ double_underflow(void **state) * * C99/C11: result will be **no greater than** DBL_MIN */ - snprintf(str, LOCAL_BUF_SIZE, "1e%d", DBL_MIN_10_EXP - 1); - assert_int_equal(pcmk__scan_double(str, &result, NULL, NULL), pcmk_rc_underflow); + buf = crm_strdup_printf("1e%d", DBL_MIN_10_EXP - 1); + assert_int_equal(pcmk__scan_double(buf, &result, NULL, NULL), + pcmk_rc_underflow); assert_true(result >= 0.0); assert_true(result <= DBL_MIN); + free(buf); - snprintf(str, LOCAL_BUF_SIZE, "-1e%d", DBL_MIN_10_EXP - 1); - assert_int_equal(pcmk__scan_double(str, &result, NULL, NULL), pcmk_rc_underflow); + buf = crm_strdup_printf("-1e%d", DBL_MIN_10_EXP - 1); + assert_int_equal(pcmk__scan_double(buf, &result, NULL, NULL), + pcmk_rc_underflow); assert_true(result <= 0.0); assert_true(result >= -DBL_MIN); + free(buf); } PCMK__UNIT_TEST(NULL, NULL, From 5ebfd70ada9fd1945d757d09af32729aeed1206f Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:32:50 -0700 Subject: [PATCH 17/49] Refactor: libcrmcommon: Avoid snprintf() in log_list_item() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- lib/common/output_log.c | 53 ++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/lib/common/output_log.c b/lib/common/output_log.c index 0a9a6b9af12..eefdf9b83e5 100644 --- a/lib/common/output_log.c +++ b/lib/common/output_log.c @@ -1,5 +1,5 @@ /* - * Copyright 2019-2024 the Pacemaker project contributors + * Copyright 2019-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -196,46 +196,39 @@ log_begin_list(pcmk__output_t *out, const char *singular_noun, const char *plura G_GNUC_PRINTF(3, 4) static void -log_list_item(pcmk__output_t *out, const char *name, const char *format, ...) { - int len = 0; +log_list_item(pcmk__output_t *out, const char *name, const char *format, ...) +{ + gsize old_len = 0; va_list ap; private_data_t *priv = NULL; - char prefix[LINE_MAX] = { 0 }; - int offset = 0; - char* buffer = NULL; + GString *buffer = g_string_sized_new(128); - pcmk__assert((out != NULL) && (out->priv != NULL)); + pcmk__assert((out != NULL) && (out->priv != NULL) && (format != NULL)); priv = out->priv; - for (GList* gIter = priv->prefixes->head; gIter; gIter = gIter->next) { - if (strcmp(prefix, "") != 0) { - offset += snprintf(prefix + offset, LINE_MAX - offset, ": %s", (char *)gIter->data); - } else { - offset = snprintf(prefix, LINE_MAX, "%s", (char *)gIter->data); - } + // Message format: [[: ...]: ]][: ] + + for (const GList *iter = priv->prefixes->head; iter != NULL; + iter = iter->next) { + + pcmk__g_strcat(buffer, (const char *) iter->data, ": ", NULL); } + if (!pcmk__str_empty(name)) { + pcmk__g_strcat(buffer, name, ": ", NULL); + } + + old_len = buffer->len; va_start(ap, format); - len = vasprintf(&buffer, format, ap); - pcmk__assert(len >= 0); + g_string_append_vprintf(buffer, format, ap); va_end(ap); - if (strcmp(buffer, "") != 0) { /* We don't want empty messages */ - if ((name != NULL) && (strcmp(name, "") != 0)) { - if (strcmp(prefix, "") != 0) { - logger(priv, "%s: %s: %s", prefix, name, buffer); - } else { - logger(priv, "%s: %s", name, buffer); - } - } else { - if (strcmp(prefix, "") != 0) { - logger(priv, "%s: %s", prefix, buffer); - } else { - logger(priv, "%s", buffer); - } - } + if (buffer->len > old_len) { + // Don't log a message with an empty body + logger(priv, "%s", buffer->str); } - free(buffer); + + g_string_free(buffer, TRUE); } static void From c16887f98ace6777a78a3d6b5ba56bdb42ea9b4a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:41:11 -0700 Subject: [PATCH 18/49] Refactor: libcrmcommon: Avoid snprintf() in pcmk__node_attr_target() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- lib/common/attrs.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/common/attrs.c b/lib/common/attrs.c index aa50831c8b8..e3d832d57c4 100644 --- a/lib/common/attrs.c +++ b/lib/common/attrs.c @@ -1,5 +1,5 @@ /* - * Copyright 2011-2024 the Pacemaker project contributors + * Copyright 2011-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -38,18 +38,22 @@ const char * pcmk__node_attr_target(const char *name) { if (name == NULL || pcmk__strcase_any_of(name, "auto", "localhost", NULL)) { - char buf[128] = OCF_RESKEY_PREFIX; - size_t offset = sizeof(OCF_RESKEY_PREFIX) - 1; + char *buf = NULL; char *target_var = crm_meta_name(PCMK_META_CONTAINER_ATTRIBUTE_TARGET); char *phys_var = crm_meta_name(PCMK__META_PHYSICAL_HOST); const char *target = NULL; const char *host_physical = NULL; - snprintf(buf + offset, sizeof(buf) - offset, "%s", target_var); + buf = crm_strdup_printf(OCF_RESKEY_PREFIX "%s", target_var); target = getenv(buf); + free(buf); - snprintf(buf + offset, sizeof(buf) - offset, "%s", phys_var); + buf = crm_strdup_printf(OCF_RESKEY_PREFIX "%s", phys_var); host_physical = getenv(buf); + free(buf); + + free(target_var); + free(phys_var); // It is important to use the name by which the scheduler knows us if (host_physical @@ -63,8 +67,6 @@ pcmk__node_attr_target(const char *name) name = host_pcmk; } } - free(target_var); - free(phys_var); // TODO? Call pcmk__cluster_local_node_name() if name == NULL // (currently would require linkage against libcrmcluster) From a4d9c924de8565d3027a2292749a2f95755ad59d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 02:49:53 -0700 Subject: [PATCH 19/49] Refactor: libcrmcommon: Avoid snprintf() in set_format_string() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- lib/common/logging.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/common/logging.c b/lib/common/logging.c index 7ba407721ee..34c57348bbe 100644 --- a/lib/common/logging.c +++ b/lib/common/logging.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -149,8 +149,6 @@ crm_log_deinit(void) pcmk__gthread_log_id = 0; } -#define FMT_MAX 256 - /*! * \internal * \brief Set the log format string based on the passed-in method @@ -174,30 +172,29 @@ set_format_string(int method, const char *daemon, pid_t use_pid, } else { // Everything else gets more detail, for advanced troubleshooting - - int offset = 0; - char fmt[FMT_MAX]; + GString *fmt = g_string_sized_new(256); if (method > QB_LOG_STDERR) { // If logging to file, prefix with timestamp, node name, daemon ID - offset += snprintf(fmt + offset, FMT_MAX - offset, - TIMESTAMP_FORMAT_SPEC " %s %-20s[%lu] ", - use_nodename, daemon, (unsigned long) use_pid); + g_string_append_printf(fmt, + TIMESTAMP_FORMAT_SPEC " %s %-20s[%lld] ", + use_nodename, daemon, (long long) use_pid); } // Add function name (in parentheses) - offset += snprintf(fmt + offset, FMT_MAX - offset, "(%%n"); + g_string_append(fmt, "(%n"); if (crm_tracing_enabled()) { // When tracing, add file and line number - offset += snprintf(fmt + offset, FMT_MAX - offset, "@%%f:%%l"); + g_string_append(fmt, "@%f:%l"); } - offset += snprintf(fmt + offset, FMT_MAX - offset, ")"); + g_string_append_c(fmt, ')'); // Add tag (if any), severity, and actual message - offset += snprintf(fmt + offset, FMT_MAX - offset, " %%g\t%%p: %%b"); + g_string_append(fmt, " %g\t%p: %b"); - CRM_LOG_ASSERT(offset > 0); - qb_log_format_set(method, fmt); + CRM_LOG_ASSERT(fmt->len > 0); + qb_log_format_set(method, fmt->str); + g_string_free(fmt, TRUE); } } From 249a3dab5dc8f7f29bbd55645fc15bc947b20fd8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 14:27:39 -0700 Subject: [PATCH 20/49] Refactor: libcrmcommon: Avoid snprintf() in pcmk__pid_active() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Ref T974 Signed-off-by: Reid Wahl --- lib/common/pid.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/common/pid.c b/lib/common/pid.c index a82d7949aa3..5082c70a504 100644 --- a/lib/common/pid.c +++ b/lib/common/pid.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -47,7 +47,8 @@ pcmk__pid_active(pid_t pid, const char *daemon) /* make sure PID hasn't been reused by another process XXX: might still be just a zombie, which could confuse decisions */ bool checked_through_kill = (rc == 0); - char exe_path[PATH_MAX], myexe_path[PATH_MAX]; + char exe_path[PATH_MAX] = { '\0', }; + char *myexe_path = NULL; rc = pcmk__procfs_pid2path(pid, exe_path, sizeof(exe_path)); if (rc != pcmk_rc_ok) { @@ -78,13 +79,14 @@ pcmk__pid_active(pid_t pid, const char *daemon) } if (daemon[0] != '/') { - rc = snprintf(myexe_path, sizeof(myexe_path), CRM_DAEMON_DIR"/%s", - daemon); + myexe_path = crm_strdup_printf(CRM_DAEMON_DIR "/%s", daemon); } else { - rc = snprintf(myexe_path, sizeof(myexe_path), "%s", daemon); + myexe_path = pcmk__str_copy(daemon); } - if (rc > 0 && rc < sizeof(myexe_path) && !strcmp(exe_path, myexe_path)) { + rc = strcmp(exe_path, myexe_path); + free(myexe_path); + if (rc == 0) { return pcmk_rc_ok; } } From d37b5e2406a4c4b52bf9509d4a4625af82109947 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 14:45:39 -0700 Subject: [PATCH 21/49] Refactor: libcrmcommon: Error-check snprintf() in pcmk_readable_score() Also avoid unnecessary strcpy() calls by returning the desired string without copying it to a buffer. The xstr() and str() macros are unnecessary if we just hard-code sizeof("-1000000"). I try to avoid using magic numbers but I don't have strong feelings. Ref T974 Signed-off-by: Reid Wahl --- lib/common/scores.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/common/scores.c b/lib/common/scores.c index 5c36fc70374..d0ea65b7f59 100644 --- a/lib/common/scores.c +++ b/lib/common/scores.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -94,28 +94,32 @@ pcmk_parse_score(const char *score_s, int *score, int default_score) * \param[in] score Score to display * * \return Pointer to static memory containing string representation of \p score - * \note Subsequent calls to this function will overwrite the returned value, so + * \note Subsequent calls to this function may overwrite the returned value, so * it should be used only in a local context such as a printf()-style * statement. */ const char * pcmk_readable_score(int score) { - // The longest possible result is "-INFINITY" - static char score_s[sizeof(PCMK_VALUE_MINUS_INFINITY)]; - if (score >= PCMK_SCORE_INFINITY) { - strcpy(score_s, PCMK_VALUE_INFINITY); + return PCMK_VALUE_INFINITY; } else if (score <= -PCMK_SCORE_INFINITY) { - strcpy(score_s, PCMK_VALUE_MINUS_INFINITY); + return PCMK_VALUE_MINUS_INFINITY; } else { - // Range is limited to +/-1000000, so no chance of overflow - snprintf(score_s, sizeof(score_s), "%d", score); - } + // https://gcc.gnu.org/onlinedocs/gcc-15.1.0/cpp/Stringizing.html +#define xstr(s) str(s) +#define str(s) #s - return score_s; + // Result must be shorter than stringized -PCMK_SCORE_INFINITY + static char score_s[sizeof(xstr(-PCMK_SCORE_INFINITY))]; +#undef xstr +#undef str + + pcmk__assert(snprintf(score_s, sizeof(score_s), "%d", score) >= 0); + return score_s; + } } /*! From 3f848cba69bce24353c3d4af3dd1d9342a4d9879 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 20 Jun 2025 16:16:58 -0700 Subject: [PATCH 22/49] Refactor: libcrmcommon: Avoid snprintf() in pcmk__set_env_option() To avoid the need to error-check snprintf() explicitly, which we have not been doing. Drop a couple of unit tests, since we're no longer using a fixed-size buffer and returning an error when it's too small. Also use CPP constants and variables where appropriate in the unit tests. Ref T974 Signed-off-by: Reid Wahl --- lib/common/options.c | 30 ++--- .../tests/options/pcmk__set_env_option_test.c | 117 +++++------------- 2 files changed, 41 insertions(+), 106 deletions(-) diff --git a/lib/common/options.c b/lib/common/options.c index b8f4943fda7..8ed3ee9ffa1 100644 --- a/lib/common/options.c +++ b/lib/common/options.c @@ -1135,38 +1135,30 @@ void pcmk__set_env_option(const char *option, const char *value, bool compat) { // @COMPAT Drop support for "HA_" options eventually - const char *const prefixes[] = {"PCMK_", "HA_"}; - char env_name[NAME_MAX]; + static const char *const prefixes[] = { "PCMK", "HA" }; CRM_CHECK(!pcmk__str_empty(option) && (strchr(option, '=') == NULL), return); for (int i = 0; i < PCMK__NELEM(prefixes); i++) { - int rv = snprintf(env_name, NAME_MAX, "%s%s", prefixes[i], option); - - if (rv < 0) { - crm_err("Failed to write %s%s to buffer: %s", prefixes[i], option, - strerror(errno)); - return; - } - - if (rv >= sizeof(env_name)) { - crm_trace("\"%s%s\" is too long", prefixes[i], option); - continue; - } + char *env_name = crm_strdup_printf("%s_%s", prefixes[i], option); + int rc = 0; if (value != NULL) { crm_trace("Setting %s to %s", env_name, value); - rv = setenv(env_name, value, 1); + rc = setenv(env_name, value, 1); } else { crm_trace("Unsetting %s", env_name); - rv = unsetenv(env_name); + rc = unsetenv(env_name); } - if (rv < 0) { - crm_err("Failed to %sset %s: %s", (value != NULL)? "" : "un", - env_name, strerror(errno)); + if (rc < 0) { + int err = errno; + + crm_err("Failed to %sset %s: %s", ((value != NULL)? "" : "un"), + env_name, strerror(err)); } + free(env_name); if (!compat && (value != NULL)) { // For set, don't proceed to HA_