fix signed integer overflow in modbus_get_float_*()

The byte assembly `i = (a << 24) | (b << 16) | (c << 8) | d` in the four
modbus_get_float_*() variants operates on uint8_t values, which promote to
signed int. When the most significant byte a is >= 0x80, `a << 24` overflows
a 32-bit signed int, which is undefined behavior.

Reported by Dominik Blain — Qreativelab Formal Verification.

Author: stephane

src/modbus-data.c

@@ -92,7 +92,11 @@ float modbus_get_float_abcd(const uint16_t *src)
     d = (src[1] >> 0) & 0xFF; // low byte of second word
 
     // we assemble 32bit integer always in abcd order via shift operations
-    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
+    // Cast to uint32_t before shifting: a..d promote to signed int, and
+    // "a << 24" with a >= 0x80 would overflow a 32-bit signed int (undefined
+    // behavior). Assembling in unsigned width keeps it well-defined. The same
+    // applies to the identical assembly in the dcba/badc/cdab variants below.
+    i = ((uint32_t) a << 24) | ((uint32_t) b << 16) | ((uint32_t) c << 8) | d;
     memcpy(&f, &i, 4);
 
     return f;
@@ -112,7 +116,7 @@ float modbus_get_float_dcba(const uint16_t *src)
     a = (src[1] >> 0) & 0xFF;
 
     // we assemble 32bit integer always in abcd order via shift operations
-    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
+    i = ((uint32_t) a << 24) | ((uint32_t) b << 16) | ((uint32_t) c << 8) | d;
     memcpy(&f, &i, 4);
 
     return f;
@@ -132,7 +136,7 @@ float modbus_get_float_badc(const uint16_t *src)
     c = (src[1] >> 0) & 0xFF;
 
     // we assemble 32bit integer always in abcd order via shift operations
-    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
+    i = ((uint32_t) a << 24) | ((uint32_t) b << 16) | ((uint32_t) c << 8) | d;
     memcpy(&f, &i, 4);
 
     return f;
@@ -152,7 +156,7 @@ float modbus_get_float_cdab(const uint16_t *src)
     b = (src[1] >> 0) & 0xFF;
 
     // we assemble 32bit integer always in abcd order via shift operations
-    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
+    i = ((uint32_t) a << 24) | ((uint32_t) b << 16) | ((uint32_t) c << 8) | d;
     memcpy(&f, &i, 4);
 
     return f;

tests/unit-test-client.c

@@ -349,6 +349,25 @@ int main(int argc, char *argv[])
     real = modbus_get_float_cdab(UT_IREAL_CDAB);
     ASSERT_TRUE(real == UT_REAL, "FAILED (%f != %f)\n", real, UT_REAL);
 
+    /* Regression: a negative float has its most significant byte >= 0x80, so
+       the "a << 24" byte assembly in modbus_get_float_*() overflows a signed
+       int (undefined behavior) unless done in unsigned width. UT_REAL is
+       positive (0x47F12000) and never exercised this path; -UT_REAL
+       (0xC7F12000) does. Build with -fsanitize=undefined to catch the UB. */
+    printf("Get negative float ABCD/DCBA/BADC/CDAB: ");
+    {
+        const uint16_t ireal_neg_abcd[] = {0xC7F1, 0x2000};
+        const uint16_t ireal_neg_dcba[] = {0x0020, 0xF1C7};
+        const uint16_t ireal_neg_badc[] = {0xF1C7, 0x0020};
+        const uint16_t ireal_neg_cdab[] = {0x2000, 0xC7F1};
+
+        ASSERT_TRUE(modbus_get_float_abcd(ireal_neg_abcd) == -UT_REAL &&
+                        modbus_get_float_dcba(ireal_neg_dcba) == -UT_REAL &&
+                        modbus_get_float_badc(ireal_neg_badc) == -UT_REAL &&
+                        modbus_get_float_cdab(ireal_neg_cdab) == -UT_REAL,
+                    "FAILED negative float get\n");
+    }
+
     printf("\nAt this point, error messages doesn't mean the test has failed\n");
 
     /** ILLEGAL DATA ADDRESS **/