From 9f140a1f421b82bc74e91ced2705c5246ab38a98 Mon Sep 17 00:00:00 2001
From: vector-of-bool <vectorofbool@gmail.com>
Date: Fri, 25 Apr 2025 11:04:59 -0600
Subject: [PATCH] Prevent multi-evaluation of arithmetic fn operands

The prior defn of `mlib_upsize_integer` would necessarily
double-evaluate the operand expression when it was of
certain types. This new definition makes use of the
ternary operator to obtain an appropriate zero
value without evaluating the operand.
---
 src/common/src/mlib/ckdint.h  | 5 +----
 src/common/src/mlib/cmp.h     | 3 ---
 src/common/src/mlib/intutil.h | 9 +++++++--
 src/common/tests/test-mlib.c  | 8 ++++++++
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/common/src/mlib/ckdint.h b/src/common/src/mlib/ckdint.h
index 4e8902ebd21..239064190b3 100644
--- a/src/common/src/mlib/ckdint.h
+++ b/src/common/src/mlib/ckdint.h
@@ -5,9 +5,6 @@
  *
  * This file implements the C23 checked-integer-arithmetic functions as macros.
  *
- * The implementation is nearly perfect: The macros necessarily evaluate the
- * operand expressions more than once, so callers should be aware of this caveat.
- *
  * The function-like macros are defined:
  *
  * - `mlib_add(Dst, L, R)` / `mlib_add(Dst, A)`
@@ -159,7 +156,7 @@ mlib_extern_c_begin ();
  * @param T A type specifier for a target integral type for the cast.
  * @param Operand The integral value to be converted.
  *
- * If the cast would result in the operand value chaning, the program will be
+ * If the cast would result in the operand value changing, the program will be
  * terminated with a diagnostic.
  */
 #define mlib_assert_narrow(T, Operand) \
diff --git a/src/common/src/mlib/cmp.h b/src/common/src/mlib/cmp.h
index a0c03947b88..ac167769c1b 100644
--- a/src/common/src/mlib/cmp.h
+++ b/src/common/src/mlib/cmp.h
@@ -46,9 +46,6 @@ enum mlib_cmp_result {
 /**
  * @brief Compare two integral values safely.
  *
- * NOTE: This macro may evaluate the operand expressions more than once! Do not
- * use expressions that are expensive or have side effects!
- *
  * This function can be called with two arguments or with three:
  *
  * - `mlib_cmp(a, b)` Returns a value of type `mlib_cmp_result`
diff --git a/src/common/src/mlib/intutil.h b/src/common/src/mlib/intutil.h
index e7bcaecd26c..db153373c2b 100644
--- a/src/common/src/mlib/intutil.h
+++ b/src/common/src/mlib/intutil.h
@@ -78,16 +78,21 @@ typedef struct mlib_upsized_integer {
  * an i64 losslessly.
  *
  * If the integer to upcast is the same size as `intmax_t`, we need to decide whether to store
- * it as unsigned. The expression `(0 & Value) - 1 < 0` will be `true` iff the operand is signed,
+ * it as unsigned. The expression `(_mlibGetZero(Values)) - 1 < 0` will be `true` iff the operand is signed,
  * otherwise false. If the operand is signed, we can safely cast to `intmax_t` (it probably already
  * is of that type), otherwise, we can to `uintmax_t` and the returned `mlib_upsized_integer` will
  * indicate that the stored value is unsigned.
  */
 #define mlib_upsize_integer(Value) \
    /* NOLINTNEXTLINE(bugprone-sizeof-expression) */ \
-   ((sizeof ((Value)) < sizeof (intmax_t) || ((0 & (Value)) - 1) < 0) \
+   ((sizeof ((Value)) < sizeof (intmax_t) || (_mlibGetZero(Value) - 1) < _mlibGetZero(Value)) \
       ? mlib_init(mlib_upsized_integer) {{(intmax_t) (Value)}, true} \
       : mlib_init(mlib_upsized_integer) {{(intmax_t) (uintmax_t) (Value)}})
+// Yield a zero value of similar-ish type to the given expression. The ternary
+// forces an integer promotion of literal zero match the type of `V`, while leaving
+// `V` unevaluated. Note that this will also promote `V` to be at least `(unsigned) int`,
+// so the zero value is only "similar" to `V`, and may be of a larger type
+#define _mlibGetZero(V) (1 ? 0 : (V))
 // clang-format on
 
 #endif // MLIB_INTUTIL_H_INCLUDED
diff --git a/src/common/tests/test-mlib.c b/src/common/tests/test-mlib.c
index b5a2848c22a..f6e8a7d5bd7 100644
--- a/src/common/tests/test-mlib.c
+++ b/src/common/tests/test-mlib.c
@@ -313,6 +313,14 @@ _test_cmp (void)
    mlib_diagnostic_pop ();
    // mlib_cmp produces the correct answer:
    ASSERT (mlib_cmp (-27, <, 20u));
+
+   {
+      // Check that we do not double-evaluate the operand expression.
+      intmax_t a = 4;
+      mlib_check (mlib_cmp (++a, ==, 5));
+      // We only increment once:
+      mlib_check (a, eq, 5);
+   }
 }
 
 static void