From d0ba2abbff2dcd4ca355f648d61fc6520f929949 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 8 Jan 2024 15:50:46 +0100 Subject: [PATCH 1/3] util: Add STATIC_ASSERT macro --- src/util.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/util.h b/src/util.h index 187bf1c5e0..154d9ebcf1 100644 --- a/src/util.h +++ b/src/util.h @@ -51,13 +51,27 @@ static void print_buf_plain(const unsigned char *buf, size_t len) { # define SECP256K1_INLINE inline # endif +/** Assert statically that expr is true. + * + * This is a statement-like macro and can only be used inside functions. + */ +#define STATIC_ASSERT(expr) do { \ + switch(0) { \ + case 0: \ + /* If expr evaluates to 0, we have two case labels "0", which is illegal. */ \ + case /* ERROR: static assertion failed */ (expr): \ + ; \ + } \ +} while(0) + /** Assert statically that expr is an integer constant expression, and run stmt. * * Useful for example to enforce that magnitude arguments are constant. */ #define ASSERT_INT_CONST_AND_DO(expr, stmt) do { \ switch(42) { \ - case /* ERROR: integer argument is not constant */ expr: \ + /* C allows only integer constant expressions as case labels. */ \ + case /* ERROR: integer argument is not constant */ (expr): \ break; \ default: ; \ } \ From e53c2d9ffc0b0096881e30e388c3fb040f35e05d Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 8 Jan 2024 15:51:15 +0100 Subject: [PATCH 2/3] Require that sizeof(secp256k1_ge_storage) == 64 This gets rid of an untested code path. Resolves #1352. secp256k1_ge_storage is a struct with two secp256k1_fe_storage fields. The C standard allows the compiler to add padding between the fields and at the end of the struct, but no sane compiler in the end would do this: The only reason to add padding is to ensure alignment, but such padding is never necessary between two fields of the same type. Similarly, secp256k1_fe_storage is a struct with a single array of uintXX_t. No padding is allowed between array elements. Again, C allows the compiler to insert padding at the end of the struct, but there's no absolute reason to do so in this case. For the uintXX_t itself, this guaranteed to have no padding bits, i.e., it's guaranteed to have exactly XX bits. So I claim that for any existing compiler in the real world, sizeof(secp256k1_ge_storage) == 64. --- src/secp256k1.c | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/src/secp256k1.c b/src/secp256k1.c index 4c11e7f0b8..15a5eede67 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -237,36 +237,25 @@ static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, } static int secp256k1_pubkey_load(const secp256k1_context* ctx, secp256k1_ge* ge, const secp256k1_pubkey* pubkey) { - if (sizeof(secp256k1_ge_storage) == 64) { - /* When the secp256k1_ge_storage type is exactly 64 byte, use its - * representation inside secp256k1_pubkey, as conversion is very fast. - * Note that secp256k1_pubkey_save must use the same representation. */ - secp256k1_ge_storage s; - memcpy(&s, &pubkey->data[0], sizeof(s)); - secp256k1_ge_from_storage(ge, &s); - } else { - /* Otherwise, fall back to 32-byte big endian for X and Y. */ - secp256k1_fe x, y; - ARG_CHECK(secp256k1_fe_set_b32_limit(&x, pubkey->data)); - ARG_CHECK(secp256k1_fe_set_b32_limit(&y, pubkey->data + 32)); - secp256k1_ge_set_xy(ge, &x, &y); - } + secp256k1_ge_storage s; + + /* We require that the secp256k1_ge_storage type is exactly 64 bytes. + * This is formally not guaranteed by the C standard, but should hold on any + * sane compiler in the real world. */ + STATIC_ASSERT(sizeof(secp256k1_ge_storage) == 64); + memcpy(&s, &pubkey->data[0], 64); + secp256k1_ge_from_storage(ge, &s); ARG_CHECK(!secp256k1_fe_is_zero(&ge->x)); return 1; } static void secp256k1_pubkey_save(secp256k1_pubkey* pubkey, secp256k1_ge* ge) { - if (sizeof(secp256k1_ge_storage) == 64) { - secp256k1_ge_storage s; - secp256k1_ge_to_storage(&s, ge); - memcpy(&pubkey->data[0], &s, sizeof(s)); - } else { - VERIFY_CHECK(!secp256k1_ge_is_infinity(ge)); - secp256k1_fe_normalize_var(&ge->x); - secp256k1_fe_normalize_var(&ge->y); - secp256k1_fe_get_b32(pubkey->data, &ge->x); - secp256k1_fe_get_b32(pubkey->data + 32, &ge->y); - } + secp256k1_ge_storage s; + + STATIC_ASSERT(sizeof(secp256k1_ge_storage) == 64); + VERIFY_CHECK(!secp256k1_ge_is_infinity(ge)); + secp256k1_ge_to_storage(&s, ge); + memcpy(&pubkey->data[0], &s, 64); } int secp256k1_ec_pubkey_parse(const secp256k1_context* ctx, secp256k1_pubkey* pubkey, const unsigned char *input, size_t inputlen) { From ba5d72d62659f9305d2be30b2ac89ce9480a0e78 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 8 Jan 2024 15:55:05 +0100 Subject: [PATCH 3/3] assumptions: Use new STATIC_ASSERT macro This also splits the big "&&" expression into separate expressions. If we ever see an assertion fail, the error message will tell it precisely which one failed. --- src/assumptions.h | 100 ++++++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 48 deletions(-) diff --git a/src/assumptions.h b/src/assumptions.h index 8ed04209e9..7961005350 100644 --- a/src/assumptions.h +++ b/src/assumptions.h @@ -19,65 +19,69 @@ reduce the odds of experiencing an unwelcome surprise. */ -struct secp256k1_assumption_checker { - /* This uses a trick to implement a static assertion in C89: a type with an array of negative size is not - allowed. */ - int dummy_array[( - /* Bytes are 8 bits. */ - (CHAR_BIT == 8) && +#if defined(__has_attribute) +# if __has_attribute(__unavailable__) +__attribute__((__unavailable__("Don't call this function. It only exists because STATIC_ASSERT cannot be used outside a function."))) +# endif +#endif +static void secp256k1_assumption_checker(void) { + /* Bytes are 8 bits. */ + STATIC_ASSERT(CHAR_BIT == 8); - /* No integer promotion for uint32_t. This ensures that we can multiply uintXX_t values where XX >= 32 - without signed overflow, which would be undefined behaviour. */ - (UINT_MAX <= UINT32_MAX) && + /* No integer promotion for uint32_t. This ensures that we can multiply uintXX_t values where XX >= 32 + without signed overflow, which would be undefined behaviour. */ + STATIC_ASSERT(UINT_MAX <= UINT32_MAX); - /* Conversions from unsigned to signed outside of the bounds of the signed type are - implementation-defined. Verify that they function as reinterpreting the lower - bits of the input in two's complement notation. Do this for conversions: - - from uint(N)_t to int(N)_t with negative result - - from uint(2N)_t to int(N)_t with negative result - - from int(2N)_t to int(N)_t with negative result - - from int(2N)_t to int(N)_t with positive result */ + /* Conversions from unsigned to signed outside of the bounds of the signed type are + implementation-defined. Verify that they function as reinterpreting the lower + bits of the input in two's complement notation. Do this for conversions: + - from uint(N)_t to int(N)_t with negative result + - from uint(2N)_t to int(N)_t with negative result + - from int(2N)_t to int(N)_t with negative result + - from int(2N)_t to int(N)_t with positive result */ - /* To int8_t. */ - ((int8_t)(uint8_t)0xAB == (int8_t)-(int8_t)0x55) && - ((int8_t)(uint16_t)0xABCD == (int8_t)-(int8_t)0x33) && - ((int8_t)(int16_t)(uint16_t)0xCDEF == (int8_t)(uint8_t)0xEF) && - ((int8_t)(int16_t)(uint16_t)0x9234 == (int8_t)(uint8_t)0x34) && + /* To int8_t. */ + STATIC_ASSERT(((int8_t)(uint8_t)0xAB == (int8_t)-(int8_t)0x55)); + STATIC_ASSERT((int8_t)(uint16_t)0xABCD == (int8_t)-(int8_t)0x33); + STATIC_ASSERT((int8_t)(int16_t)(uint16_t)0xCDEF == (int8_t)(uint8_t)0xEF); + STATIC_ASSERT((int8_t)(int16_t)(uint16_t)0x9234 == (int8_t)(uint8_t)0x34); - /* To int16_t. */ - ((int16_t)(uint16_t)0xBCDE == (int16_t)-(int16_t)0x4322) && - ((int16_t)(uint32_t)0xA1B2C3D4 == (int16_t)-(int16_t)0x3C2C) && - ((int16_t)(int32_t)(uint32_t)0xC1D2E3F4 == (int16_t)(uint16_t)0xE3F4) && - ((int16_t)(int32_t)(uint32_t)0x92345678 == (int16_t)(uint16_t)0x5678) && + /* To int16_t. */ + STATIC_ASSERT((int16_t)(uint16_t)0xBCDE == (int16_t)-(int16_t)0x4322); + STATIC_ASSERT((int16_t)(uint32_t)0xA1B2C3D4 == (int16_t)-(int16_t)0x3C2C); + STATIC_ASSERT((int16_t)(int32_t)(uint32_t)0xC1D2E3F4 == (int16_t)(uint16_t)0xE3F4); + STATIC_ASSERT((int16_t)(int32_t)(uint32_t)0x92345678 == (int16_t)(uint16_t)0x5678); - /* To int32_t. */ - ((int32_t)(uint32_t)0xB2C3D4E5 == (int32_t)-(int32_t)0x4D3C2B1B) && - ((int32_t)(uint64_t)0xA123B456C789D012ULL == (int32_t)-(int32_t)0x38762FEE) && - ((int32_t)(int64_t)(uint64_t)0xC1D2E3F4A5B6C7D8ULL == (int32_t)(uint32_t)0xA5B6C7D8) && - ((int32_t)(int64_t)(uint64_t)0xABCDEF0123456789ULL == (int32_t)(uint32_t)0x23456789) && + /* To int32_t. */ + STATIC_ASSERT((int32_t)(uint32_t)0xB2C3D4E5 == (int32_t)-(int32_t)0x4D3C2B1B); + STATIC_ASSERT((int32_t)(uint64_t)0xA123B456C789D012ULL == (int32_t)-(int32_t)0x38762FEE); + STATIC_ASSERT((int32_t)(int64_t)(uint64_t)0xC1D2E3F4A5B6C7D8ULL == (int32_t)(uint32_t)0xA5B6C7D8); + STATIC_ASSERT((int32_t)(int64_t)(uint64_t)0xABCDEF0123456789ULL == (int32_t)(uint32_t)0x23456789); - /* To int64_t. */ - ((int64_t)(uint64_t)0xB123C456D789E012ULL == (int64_t)-(int64_t)0x4EDC3BA928761FEEULL) && + /* To int64_t. */ + STATIC_ASSERT((int64_t)(uint64_t)0xB123C456D789E012ULL == (int64_t)-(int64_t)0x4EDC3BA928761FEEULL); #if defined(SECP256K1_INT128_NATIVE) - ((int64_t)(((uint128_t)0xA1234567B8901234ULL << 64) + 0xC5678901D2345678ULL) == (int64_t)-(int64_t)0x3A9876FE2DCBA988ULL) && - (((int64_t)(int128_t)(((uint128_t)0xB1C2D3E4F5A6B7C8ULL << 64) + 0xD9E0F1A2B3C4D5E6ULL)) == (int64_t)(uint64_t)0xD9E0F1A2B3C4D5E6ULL) && - (((int64_t)(int128_t)(((uint128_t)0xABCDEF0123456789ULL << 64) + 0x0123456789ABCDEFULL)) == (int64_t)(uint64_t)0x0123456789ABCDEFULL) && + STATIC_ASSERT((int64_t)(((uint128_t)0xA1234567B8901234ULL << 64) + 0xC5678901D2345678ULL) == (int64_t)-(int64_t)0x3A9876FE2DCBA988ULL); + STATIC_ASSERT(((int64_t)(int128_t)(((uint128_t)0xB1C2D3E4F5A6B7C8ULL << 64) + 0xD9E0F1A2B3C4D5E6ULL)) == (int64_t)(uint64_t)0xD9E0F1A2B3C4D5E6ULL); + STATIC_ASSERT(((int64_t)(int128_t)(((uint128_t)0xABCDEF0123456789ULL << 64) + 0x0123456789ABCDEFULL)) == (int64_t)(uint64_t)0x0123456789ABCDEFULL); - /* To int128_t. */ - ((int128_t)(((uint128_t)0xB1234567C8901234ULL << 64) + 0xD5678901E2345678ULL) == (int128_t)(-(int128_t)0x8E1648B3F50E80DCULL * 0x8E1648B3F50E80DDULL + 0x5EA688D5482F9464ULL)) && + /* To int128_t. */ + STATIC_ASSERT((int128_t)(((uint128_t)0xB1234567C8901234ULL << 64) + 0xD5678901E2345678ULL) == (int128_t)(-(int128_t)0x8E1648B3F50E80DCULL * 0x8E1648B3F50E80DDULL + 0x5EA688D5482F9464ULL)); #endif - /* Right shift on negative signed values is implementation defined. Verify that it - acts as a right shift in two's complement with sign extension (i.e duplicating - the top bit into newly added bits). */ - ((((int8_t)0xE8) >> 2) == (int8_t)(uint8_t)0xFA) && - ((((int16_t)0xE9AC) >> 4) == (int16_t)(uint16_t)0xFE9A) && - ((((int32_t)0x937C918A) >> 9) == (int32_t)(uint32_t)0xFFC9BE48) && - ((((int64_t)0xA8B72231DF9CF4B9ULL) >> 19) == (int64_t)(uint64_t)0xFFFFF516E4463BF3ULL) && + /* Right shift on negative signed values is implementation defined. Verify that it + acts as a right shift in two's complement with sign extension (i.e duplicating + the top bit into newly added bits). */ + STATIC_ASSERT((((int8_t)0xE8) >> 2) == (int8_t)(uint8_t)0xFA); + STATIC_ASSERT((((int16_t)0xE9AC) >> 4) == (int16_t)(uint16_t)0xFE9A); + STATIC_ASSERT((((int32_t)0x937C918A) >> 9) == (int32_t)(uint32_t)0xFFC9BE48); + STATIC_ASSERT((((int64_t)0xA8B72231DF9CF4B9ULL) >> 19) == (int64_t)(uint64_t)0xFFFFF516E4463BF3ULL); #if defined(SECP256K1_INT128_NATIVE) - ((((int128_t)(((uint128_t)0xCD833A65684A0DBCULL << 64) + 0xB349312F71EA7637ULL)) >> 39) == (int128_t)(((uint128_t)0xFFFFFFFFFF9B0674ULL << 64) + 0xCAD0941B79669262ULL)) && + STATIC_ASSERT((((int128_t)(((uint128_t)0xCD833A65684A0DBCULL << 64) + 0xB349312F71EA7637ULL)) >> 39) == (int128_t)(((uint128_t)0xFFFFFFFFFF9B0674ULL << 64) + 0xCAD0941B79669262ULL)); #endif - 1) * 2 - 1]; -}; + + /* This function is not supposed to be called. */ + VERIFY_CHECK(0); +} #endif /* SECP256K1_ASSUMPTIONS_H */