From 7585848ac9fbef609a1520263ef085dbb08869b5 Mon Sep 17 00:00:00 2001 From: jun <83899451+zeichensystem@users.noreply.github.com> Date: Thu, 15 May 2025 13:24:29 +0200 Subject: [PATCH] Prevent more potential int promotion bugs --- src/guf_hash.h | 8 +++---- src/guf_math.h | 16 +++++++------- src/guf_math_ckdint.h | 1 - src/guf_rand.h | 46 ++++++++++++++++++++-------------------- src/test/test_ckdint.cpp | 26 +++++++++++++++++++++-- todo.txt | 3 ++- 6 files changed, 61 insertions(+), 39 deletions(-) diff --git a/src/guf_hash.h b/src/guf_hash.h index 1ab380a..ca34a4d 100644 --- a/src/guf_hash.h +++ b/src/guf_hash.h @@ -60,8 +60,8 @@ GUF_HASH_KWRDS uint32_t guf_hash32(const void *data, ptrdiff_t num_bytes, uint32 const unsigned char *data_bytes = (const unsigned char*)data; // This does not break strict-aliasing rules I think... const uint32_t FNV_32_PRIME = UINT32_C(16777619); for (ptrdiff_t i = 0; i < num_bytes; ++i) { - hash ^= data_bytes[i]; - hash *= FNV_32_PRIME; + hash ^= 1u * data_bytes[i]; + hash *= 1u * FNV_32_PRIME; } return hash; } @@ -74,8 +74,8 @@ GUF_HASH_KWRDS uint32_t guf_hash32(const void *data, ptrdiff_t num_bytes, uint32 const unsigned char *data_bytes = (const unsigned char*)data; // This does not break strict-aliasing rules I think... const uint64_t FNV_64_PRIME = UINT64_C(1099511628211); for (ptrdiff_t i = 0; i < num_bytes; ++i) { - hash ^= data_bytes[i]; - hash *= FNV_64_PRIME; + hash ^= 1u * data_bytes[i]; + hash *= 1u * FNV_64_PRIME; } return hash; } diff --git a/src/guf_math.h b/src/guf_math.h index b9e68f2..cf9cfa7 100644 --- a/src/guf_math.h +++ b/src/guf_math.h @@ -16,8 +16,8 @@ #define GUF_MAX_F64_LT_ONE (1.0 - DBL_EPSILON/FLT_RADIX) // Rotate left. -static inline uint64_t guf_rotl_u64(uint64_t x, int k) {return (x << k) | (x >> (64 - k));} -static inline uint32_t guf_rotl_u32(uint32_t x, int k) {return (x << k) | (x >> (32 - k));} +static inline uint64_t guf_rotl_u64(uint64_t x, int k) {return (1u*x << k) | (1u*x >> (64 - k));} +static inline uint32_t guf_rotl_u32(uint32_t x, int k) {return (1u*x << k) | (1u*x >> (32 - k));} // Signed min, max, clamp functions (generated with libguf/tools/min_max_clamp-gen.py) @@ -114,12 +114,12 @@ static inline int64_t guf_abs_i64(int64_t x) {if (x >= 0) {return x;} static inline ptrdiff_t guf_abs_ptrdiff(ptrdiff_t x) {if (x >= 0) {return x;} GUF_ASSERT_RELEASE(-PTRDIFF_MAX == PTRDIFF_MIN || x > PTRDIFF_MIN); return -x;} // abs functions with unsigned result functions (cannot fail) -static inline unsigned guf_uabs_int(int x) {if (x >= 0) {return x;} else if (x == INT_MIN && -INT_MAX != INT_MIN) {return (unsigned)INT_MAX + 1;} else {return -x;}} -static inline uint8_t guf_uabs_i8(int8_t x) {if (x >= 0) {return x;} else if (x == INT8_MIN && -INT8_MAX != INT8_MIN) {return (uint8_t)INT8_MAX + 1;} else {return -x;}} -static inline uint16_t guf_uabs_i16(int16_t x) {if (x >= 0) {return x;} else if (x == INT16_MIN && -INT16_MAX != INT16_MIN) {return (uint16_t)INT16_MAX + 1;} else {return -x;}} -static inline uint32_t guf_uabs_i32(int32_t x) {if (x >= 0) {return x;} else if (x == INT32_MIN && -INT32_MAX != INT32_MIN) {return (uint32_t)INT32_MAX + 1;} else {return -x;}} -static inline uint64_t guf_uabs_i64(int64_t x) {if (x >= 0) {return x;} else if (x == INT64_MIN && -INT64_MAX != INT64_MIN) {return (uint64_t)INT64_MAX + 1;} else {return -x;}} -static inline size_t guf_uabs_ptrdiff_t(ptrdiff_t x) {if (x >= 0) {return x;} else if (x == PTRDIFF_MIN && -PTRDIFF_MAX != PTRDIFF_MIN) {return (size_t)PTRDIFF_MAX + 1;} else {return -x;}} +static inline unsigned guf_uabs_int(int x) {if (x >= 0) {return x;} else if (x == INT_MIN && -INT_MAX != INT_MIN) {return (unsigned)INT_MAX + 1u;} else {return -x;}} +static inline uint8_t guf_uabs_i8(int8_t x) {if (x >= 0) {return x;} else if (x == INT8_MIN && -INT8_MAX != INT8_MIN) {return (uint8_t)INT8_MAX + 1u;} else {return -x;}} +static inline uint16_t guf_uabs_i16(int16_t x) {if (x >= 0) {return x;} else if (x == INT16_MIN && -INT16_MAX != INT16_MIN) {return (uint16_t)INT16_MAX + 1u;} else {return -x;}} +static inline uint32_t guf_uabs_i32(int32_t x) {if (x >= 0) {return x;} else if (x == INT32_MIN && -INT32_MAX != INT32_MIN) {return (uint32_t)INT32_MAX + 1u;} else {return -x;}} +static inline uint64_t guf_uabs_i64(int64_t x) {if (x >= 0) {return x;} else if (x == INT64_MIN && -INT64_MAX != INT64_MIN) {return (uint64_t)INT64_MAX + 1u;} else {return -x;}} +static inline size_t guf_uabs_ptrdiff_t(ptrdiff_t x) {if (x >= 0) {return x;} else if (x == PTRDIFF_MIN && -PTRDIFF_MAX != PTRDIFF_MIN) {return (size_t)PTRDIFF_MAX + 1u;} else {return -x;}} // absdiff functions with unsigned result (cannot fail) static inline unsigned char guf_absdiff_char(char a, char b) {return a > b ? (unsigned char)a - (unsigned char)b : (unsigned char)b - (unsigned char)a;} diff --git a/src/guf_math_ckdint.h b/src/guf_math_ckdint.h index 2f7f6da..ca3cea2 100644 --- a/src/guf_math_ckdint.h +++ b/src/guf_math_ckdint.h @@ -68,7 +68,6 @@ cf. https://stackoverflow.com/questions/27001604/32-bit-unsigned-multiply-on-64-bit-causing-undefined-behavior (last-retrieved 2025-05-15) */ - #if defined(GUF_MATH_CKDINT_IMPL_STATIC) #define GUF_MATH_CKDINT_KWRDS static inline #else diff --git a/src/guf_rand.h b/src/guf_rand.h index 2935e25..e3c1177 100644 --- a/src/guf_rand.h +++ b/src/guf_rand.h @@ -239,10 +239,10 @@ GUF_RAND_KWRDS double guf_rand32_normal_sample_one_f64(guf_rand32_state *state, GUF_RAND_KWRDS uint64_t guf_rand_splitmix64(uint64_t *state) { GUF_ASSERT(state); - uint64_t z = ((*state) += 0x9e3779b97f4a7c15); - z = (z ^ (z >> 30)) * 0xbf58476d1ce4e5b9; - z = (z ^ (z >> 27)) * 0x94d049bb133111eb; - return z ^ (z >> 31); + uint64_t z = ((*state) += 1u * 0x9e3779b97f4a7c15); + z = (z ^ (z >> 30u)) * 0xbf58476d1ce4e5b9; + z = (z ^ (z >> 27u)) * 0x94d049bb133111eb; + return z ^ (z >> 31u); } #endif @@ -253,10 +253,10 @@ GUF_RAND_KWRDS double guf_rand32_normal_sample_one_f64(guf_rand32_state *state, GUF_RAND_KWRDS uint32_t guf_rand_splitmix32(uint32_t *state) { GUF_ASSERT(state); - uint32_t z = (*state += 0x9e3779b9); - z = (z ^ (z >> 16)) * 0x85ebca6b; - z = (z ^ (z >> 13)) * 0xc2b2ae35; - return z ^ (z >> 16); + uint32_t z = (*state += 1u * 0x9e3779b9); + z = (z ^ (z >> 16u)) * 0x85ebca6b; + z = (z ^ (z >> 13u)) * 0xc2b2ae35; + return z ^ (z >> 16u); } GUF_RAND_KWRDS void guf_rand32_state_init(guf_rand32_state *state, uint32_t seed) @@ -306,8 +306,8 @@ GUF_RAND_KWRDS uint32_t guf_rand32_u32(guf_rand32_state *state) xoshiro128** 1.1 (public domain) written in 2018 by David Blackman and Sebastiano Vigna (vigna@acm.org) cf. https://prng.di.unimi.it/xoshiro128starstar.c (last-retrieved 2025-02-11) */ - const uint32_t result = guf_rotl_u32(state->s[1] * 5, 7) * 9; - const uint32_t t = state->s[1] << 9; + const uint32_t result = guf_rotl_u32(state->s[1] * 5u, 7) * 9u; + const uint32_t t = state->s[1] << 9u; state->s[2] ^= state->s[0]; state->s[3] ^= state->s[1]; state->s[1] ^= state->s[2]; @@ -320,7 +320,7 @@ GUF_RAND_KWRDS uint32_t guf_rand32_u32(guf_rand32_state *state) #ifdef UINT64_MAX GUF_RAND_KWRDS uint32_t guf_rand64_u32(guf_rand64_state *state) { - return (uint32_t)(guf_rand64_u64(state) >> 32); + return (uint32_t)(guf_rand64_u64(state) >> 32u); } #endif @@ -345,8 +345,8 @@ GUF_RAND_KWRDS uint32_t guf_rand_u32(guf_randstate *state) xoshiro256** 1.0 (public domain) written in 2018 by David Blackman and Sebastiano Vigna (vigna@acm.org) cf. https://prng.di.unimi.it/xoshiro256starstar.c (last-retrieved 2025-02-11) */ - const uint64_t result = guf_rotl_u64(state->s[1] * 5, 7) * 9; - const uint64_t t = state->s[1] << 17; + const uint64_t result = guf_rotl_u64(state->s[1] * 5u, 7) * 9u; + const uint64_t t = state->s[1] << 17u; state->s[2] ^= state->s[0]; state->s[3] ^= state->s[1]; state->s[1] ^= state->s[2]; @@ -368,9 +368,9 @@ GUF_RAND_KWRDS uint_least64_t guf_rand32_u64(guf_rand32_state *state) const uint32_t lower_bits = guf_rand32_u32(state); const uint32_t upper_bits = guf_rand32_u32(state); #ifdef UINT64_MAX - return ((uint64_t)upper_bits << 32) | (uint64_t)lower_bits; // TODO: not sure if that's a good idea... + return ((uint64_t)upper_bits << 32u) | (uint64_t)lower_bits; // TODO: not sure if that's a good idea... #else - return ((uint_least64_t)upper_bits << 32) | (uint_least64_t)lower_bits; // TODO: not sure if that's a good idea... + return ((uint_least64_t)upper_bits << 32u) | (uint_least64_t)lower_bits; // TODO: not sure if that's a good idea... #endif } @@ -467,33 +467,33 @@ GUF_RAND_KWRDS void guf_randstate_jump(guf_randstate *state) GUF_RAND_KWRDS double guf_rand64_f64(guf_rand64_state *state) { // cf. https://prng.di.unimi.it/ and https://dotat.at/@/2023-06-23-random-double.html (last-retrieved 2025-02-11) - return (guf_rand64_u64(state) >> 11) * 0x1.0p-53; // 11 == 64 - 53 (double has a 53-bit mantissa/significand) + return (guf_rand64_u64(state) >> 11u) * 0x1.0p-53; // 11 == 64 - 53 (double has a 53-bit mantissa/significand) } #endif GUF_RAND_KWRDS double guf_rand32_f64(guf_rand32_state *state) { // cf. https://prng.di.unimi.it/ and https://dotat.at/@/2023-06-23-random-double.html (last-retrieved 2025-02-11) - return (guf_rand32_u64(state) >> 11) * 0x1.0p-53; // 11 == 64 - 53 (double has a 53-bit mantissa/significand) + return (guf_rand32_u64(state) >> 11u) * 0x1.0p-53; // 11 == 64 - 53 (double has a 53-bit mantissa/significand) } // Generate double in the unit interval [0, 1) GUF_RAND_KWRDS double guf_rand_f64(guf_randstate *state) { // cf. https://prng.di.unimi.it/ and https://dotat.at/@/2023-06-23-random-double.html (last-retrieved 2025-02-11) - return (guf_rand_u64(state) >> 11) * 0x1.0p-53; // 11 == 64 - 53 (double has a 53-bit mantissa/significand) + return (guf_rand_u64(state) >> 11u) * 0x1.0p-53; // 11 == 64 - 53 (double has a 53-bit mantissa/significand) } #ifdef UINT64_MAX GUF_RAND_KWRDS float guf_rand64_f32(guf_rand64_state *state) { - return (guf_rand64_u64(state) >> 40) * 0x1.0p-24f; // 40 == 64 - 24; (float has a 24-bit mantissa/significand) + return (guf_rand64_u64(state) >> 40u) * 0x1.0p-24f; // 40 == 64 - 24; (float has a 24-bit mantissa/significand) } #endif GUF_RAND_KWRDS float guf_rand32_f32(guf_rand32_state *state) { - return (guf_rand32_u32(state) >> 8) * 0x1.0p-24f; // 8 == 32 - 24; (float has a 24-bit mantissa/significand) + return (guf_rand32_u32(state) >> 8u) * 0x1.0p-24f; // 8 == 32 - 24; (float has a 24-bit mantissa/significand) } // Generate float in the unit interval [0, 1) @@ -682,7 +682,7 @@ GUF_RAND_KWRDS int32_t guf_rand32_range_i32(guf_rand32_state *state, int32_t min return min; } - const uint32_t rand_max_i32 = UINT32_MAX >> 1; // 2^31 - 1 (== INT32_MAX) + const uint32_t rand_max_i32 = UINT32_MAX >> 1u; // 2^31 - 1 (== INT32_MAX) const uint32_t delta = guf_absdiff_i32(max, min); if (delta > rand_max_i32) { @@ -709,7 +709,7 @@ GUF_RAND_KWRDS int32_t guf_rand32_range_i32(guf_rand32_state *state, int32_t min */ uint32_t step; do { - step = guf_rand32_u32(state) >> 1; // [0, 2^31 - 1] + step = guf_rand32_u32(state) >> 1u; // [0, 2^31 - 1] } while (step >= limit); step = step / bin_size; @@ -778,7 +778,7 @@ GUF_RAND_KWRDS uint32_t guf_randrange_u32(guf_randstate *state, uint32_t min, ui return min; } - const uint64_t rand_max_i64 = UINT64_MAX >> 1; // 2^63 - 1 (== INT64_MAX) + const uint64_t rand_max_i64 = UINT64_MAX >> 1u; // 2^63 - 1 (== INT64_MAX) const uint64_t delta = guf_absdiff_i64(max, min); if (delta > rand_max_i64) { diff --git a/src/test/test_ckdint.cpp b/src/test/test_ckdint.cpp index b10d157..69b85de 100644 --- a/src/test/test_ckdint.cpp +++ b/src/test/test_ckdint.cpp @@ -154,6 +154,12 @@ void CkdIntTest::test_ckd() TEST_CHECK(guf_wrapping_mul_i8(5, 42, &mul_i8_res) == GUF_MATH_CKD_OVERFLOW_POS); TEST_CHECK(mul_i8_res == -46); + int16_t mul_i16_res = -1245; + TEST_CHECK(guf_wrapping_mul_i16(32767, 2, &mul_i16_res) == GUF_MATH_CKD_OVERFLOW_POS); + TEST_CHECK(mul_i16_res == -2); + mul_i16_res = -1245; + TEST_CHECK(guf_wrapping_mul_i16(-32767, 2, &mul_i16_res) == GUF_MATH_CKD_OVERFLOW_NEG); + TEST_CHECK(mul_i16_res == 2); /* // https://play.rust-lang.org/?version=stable&mode=debug&edition=2024 @@ -166,10 +172,23 @@ void CkdIntTest::test_ckd() } */ - int32_t mul_i32_res = -1; + int32_t mul_i32_res = -12345; + TEST_CHECK(guf_wrapping_mul_i32(INT32_MAX, 2, &mul_i32_res) == GUF_MATH_CKD_OVERFLOW_POS); + TEST_CHECK(mul_i32_res == -2); + mul_i32_res = -12345; + TEST_CHECK(guf_wrapping_mul_i32(2, INT32_MAX, &mul_i32_res) == GUF_MATH_CKD_OVERFLOW_POS); + TEST_CHECK(mul_i32_res == -2); + + mul_i32_res = -12345; + TEST_CHECK(guf_wrapping_mul_i32(INT32_MAX, -2, &mul_i32_res) == GUF_MATH_CKD_OVERFLOW_NEG); + TEST_CHECK(mul_i32_res == 2); + mul_i32_res = -12345; + TEST_CHECK(guf_wrapping_mul_i32(-2, INT32_MAX, &mul_i32_res) == GUF_MATH_CKD_OVERFLOW_NEG); + TEST_CHECK(mul_i32_res == 2); + TEST_CHECK(guf_wrapping_mul_i32(42002718, 314159265, &mul_i32_res) == GUF_MATH_CKD_OVERFLOW_POS); TEST_CHECK(mul_i32_res == -972735522); - mul_i32_res = -1; + mul_i32_res = -12345; TEST_CHECK(guf_wrapping_mul_i32(314159265, 42002718, &mul_i32_res) == GUF_MATH_CKD_OVERFLOW_POS); TEST_CHECK(mul_i32_res == -972735522); @@ -204,11 +223,14 @@ void CkdIntTest::test_ckd() ptrdiff_t ptrdiff_res = -1234; TEST_CHECK(guf_saturating_add_ptrdiff_t(PTRDIFF_MAX, 1, &ptrdiff_res) == GUF_MATH_CKD_OVERFLOW_POS); TEST_CHECK(ptrdiff_res == PTRDIFF_MAX); + ptrdiff_res = -1234; TEST_CHECK(guf_saturating_add_ptrdiff_t(PTRDIFF_MIN, -1, &ptrdiff_res) == GUF_MATH_CKD_OVERFLOW_NEG); TEST_CHECK(ptrdiff_res == PTRDIFF_MIN); + ptrdiff_res = -1234; TEST_CHECK(guf_saturating_mul_ptrdiff_t(PTRDIFF_MAX, 2, &ptrdiff_res) == GUF_MATH_CKD_OVERFLOW_POS); TEST_CHECK(ptrdiff_res == PTRDIFF_MAX); + ptrdiff_res = -1234; TEST_CHECK(guf_saturating_mul_ptrdiff_t(PTRDIFF_MIN, 2, &ptrdiff_res) == GUF_MATH_CKD_OVERFLOW_NEG); TEST_CHECK(ptrdiff_res == PTRDIFF_MIN); } diff --git a/todo.txt b/todo.txt index 635e3ff..dda9900 100644 --- a/todo.txt +++ b/todo.txt @@ -2,6 +2,7 @@ - sort: add cpp #ifdef to remove restrict from declaration - guf_wrapping_mul_TYPE: Not 100 % sure if it does not depend on implementation defined behaviour, but it shouldn't +- https://en.cppreference.com/w/c/types/integer (apparently the signed fixed width integer types are guaranteed to be two's complement...) - tests for guf_dict with GUF_DICT_64_BIT_IDX (and also hash32/hash64); maybe pass kv_type to insert to avoid copy - dict elems shrink to fit; allow to pass GUF_DBUF_USE_GROWTH_FAC_ONE_POINT_FIVE; start capacity (for elems and kv_indices?) @@ -12,4 +13,4 @@ - no guf_init.h - unicode normalisation -- handle right-to-left text properly \ No newline at end of file +- handle right-to-left text properly