Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1480: Get rid of untested sizeof(secp256…
Browse files Browse the repository at this point in the history 
…k1_ge_storage) == 64 code path

ba5d72d assumptions: Use new STATIC_ASSERT macro (Tim Ruffing)
e53c2d9 Require that sizeof(secp256k1_ge_storage) == 64 (Tim Ruffing)
d0ba2ab util: Add STATIC_ASSERT macro (Tim Ruffing)

Pull request description:

  This gets rid of an untested code path. Resolves bitcoin-core/secp256k1#1352.

  This is a bit opinionated in the sense that it adds a static assertion where it's needed in `secp256k1_pubkey_save` and `secp256k1_pubkey_load`. I think this is justified in this case. It helps the reviewer verify that these functions are correct.

  See individual commit messages.

ACKs for top commit:
  sipa:
    utACK ba5d72d
  jonasnick:
    ACK ba5d72d

Tree-SHA512: 2553c0610b62bcda6d4ef26eb26b5b2e07acf723bcd299691a2d02da57af22b8763f63c2d4adb17d30de8825b6157be6e4f0398147854fbabdf8b865fb0b5c88
  • Loading branch information
@real-or-random
real-or-random committed Jan 9, 2024
2 parents 74b7c3b + ba5d72d commit a9db9f2
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 74 deletions.
100 changes: 52 additions & 48 deletions src/assumptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
39 changes: 14 additions & 25 deletions src/secp256k1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
16 changes: 15 additions & 1 deletion src/util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: ; \
} \
Expand Down

0 comments on commit a9db9f2

Please sign in to comment.