utils: Reimplement ver2str() for sanity
The dense mix of print_version_field(), snprintf() with its tricky
return semantics, and the tricky POINTER_MOVE() macro are replaced by an
implementation where each statement directly inserts one character into
the buffer while preventing buffer overflow.
While we're reworking ver2str(), change the return type to avoid
clang-tidy's bugprone-narrowing-conversion diagnostic. This is an
API/ABI break, but it's one with low impact: The test suite declares the
variable holding the return type as `auto`, while the one ver2str() call
in the entire openbmc github org, in pldmd, immediately discards the
return value (it is never assigned).
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Change-Id: I11708a32019a36ea0d31229f6c91c8a75f7f22d0
diff --git a/include/libpldm/utils.h b/include/libpldm/utils.h
index 35b039b..9b7bd9f 100644
--- a/include/libpldm/utils.h
+++ b/include/libpldm/utils.h
@@ -9,6 +9,7 @@
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
+#include <sys/types.h>
/** @struct variable_field
*
@@ -38,11 +39,14 @@
/** @brief Convert ver32_t to string
* @param[in] version - Pointer to ver32_t
* @param[out] buffer - Pointer to the buffer
- * @param[in] buffer_size - Size of the buffer
- * @return The number of characters(excluding the null byte) or negative if
- * error is encountered
+ * @param[in] buffer_size - Size of the buffer, up to SSIZE_MAX
+ * @return The number of characters written to the buffer (excluding the null
+ * byte). The converted string may be truncated, and truncation is not
+ * considered an error. The result is negative if invalid arguments are supplied
+ * (NULL values for required pointers or the buffer size is beyond a
+ * representable range).
*/
-int ver2str(const ver32_t *version, char *buffer, size_t buffer_size);
+ssize_t ver2str(const ver32_t *version, char *buffer, size_t buffer_size);
/** @brief Convert bcd number(uint8_t) to decimal
* @param[in] bcd - bcd number
diff --git a/src/utils.c b/src/utils.c
index b4b930b..b78ffe5 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -1,5 +1,6 @@
#include "utils.h"
#include "base.h"
+#include <limits.h>
#include <stdio.h>
/** CRC32 code derived from work by Gary S. Brown.
@@ -98,50 +99,56 @@
return crc;
}
-static int print_version_field(uint8_t bcd, char *buffer, size_t buffer_size)
+#define BCD_H(v) (((v) >> 4) & 0xf)
+#define BCD_L(v) ((v)&0xf)
+#define AS_CHAR(digit) ((digit) + '0')
+#define INSERT_CHAR(c, b, n) \
+ { \
+ if (n > 1) { \
+ *(b)++ = (c); \
+ (n)--; \
+ } \
+ }
+#define INSERT_INT(i, b, n) INSERT_CHAR(AS_CHAR(i), (b), (n))
+ssize_t ver2str(const ver32_t *version, char *buffer, size_t buffer_size)
{
- int v;
- if (bcd == 0xff)
- return 0;
- if ((bcd & 0xf0) == 0xf0) {
- v = bcd & 0x0f;
- return snprintf(buffer, buffer_size, "%d", v);
- }
- v = ((bcd >> 4) * 10) + (bcd & 0x0f);
- return snprintf(buffer, buffer_size, "%02d", v);
-}
+ ssize_t remaining;
+ char *cursor;
-#define POINTER_MOVE(rc, buffer, buffer_size, original_size) \
- do { \
- if (rc < 0) \
- return rc; \
- if ((size_t)rc >= buffer_size) \
- return original_size - 1; \
- buffer += rc; \
- buffer_size -= rc; \
- } while (0)
+ if (!version || !buffer)
+ return -1;
-int ver2str(const ver32_t *version, char *buffer, size_t buffer_size)
-{
- int rc;
- size_t original_size = buffer_size;
- rc = print_version_field(version->major, buffer, buffer_size);
- POINTER_MOVE(rc, buffer, buffer_size, original_size);
- rc = snprintf(buffer, buffer_size, ".");
- POINTER_MOVE(rc, buffer, buffer_size, original_size);
- rc = print_version_field(version->minor, buffer, buffer_size);
- POINTER_MOVE(rc, buffer, buffer_size, original_size);
- if (version->update != 0xff) {
- rc = snprintf(buffer, buffer_size, ".");
- POINTER_MOVE(rc, buffer, buffer_size, original_size);
- rc = print_version_field(version->update, buffer, buffer_size);
- POINTER_MOVE(rc, buffer, buffer_size, original_size);
+ if (!buffer_size)
+ return -1;
+
+ if (buffer_size > SSIZE_MAX)
+ return -1;
+
+ cursor = buffer;
+ remaining = (ssize_t)buffer_size;
+
+ if (version->major < 0xf0)
+ INSERT_INT(BCD_H(version->major), cursor, remaining)
+ INSERT_INT(BCD_L(version->major), cursor, remaining);
+ INSERT_CHAR('.', cursor, remaining);
+
+ if (version->minor < 0xf0)
+ INSERT_INT(BCD_H(version->minor), cursor, remaining);
+ INSERT_INT(BCD_L(version->minor), cursor, remaining);
+
+ if (version->update < 0xff) {
+ INSERT_CHAR('.', cursor, remaining);
+ if (version->update < 0xf0)
+ INSERT_INT(BCD_H(version->update), cursor, remaining);
+ INSERT_INT(BCD_L(version->update), cursor, remaining);
}
- if (version->alpha != 0) {
- rc = snprintf(buffer, buffer_size, "%c", version->alpha);
- POINTER_MOVE(rc, buffer, buffer_size, original_size);
- }
- return original_size - buffer_size;
+
+ if (version->alpha)
+ INSERT_CHAR(version->alpha, cursor, remaining);
+
+ *cursor = '\0';
+
+ return (ssize_t)buffer_size - remaining;
}
uint8_t bcd2dec8(uint8_t bcd)