From e53c2d9ffc0b0096881e30e388c3fb040f35e05d Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Mon, 8 Jan 2024 15:51:15 +0100
Subject: [PATCH] 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 4c11e7f0b..15a5eede6 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) {