The sizeof(secp256k1_ge_storage) != 64 path in secp256k1_pubkey_load and secp256k1_pubkey_save is not tested currently because we’re not aware of a compiler/platform where this condition is true.
This is my analysis from #1349:
I lean towards getting rid of the code path entirely because I don’t think it’s relevant in the real world:
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. (If this was an array with two members instead of a struct with two members, then the alignment requirements would be the same, but no padding would be allowed.)
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, and we could assert this in assumptions.h.
Alternatives:
- Get rid of the optimization and just never rely on the fact that
sizeof(secp256k1_ge_storage) == 64(but that’s slower) - Switch
secp256k1_ge_storageandsecp256k1_fe_storageto be actual array types so that the compiler is not allowed to add padding (but that’s pretty bad C style:sizeof()has different semantics on array types,=assignment is not possible, returning and passing by value is not possible, …) - Have a test override (but that’s complex and makes testing harder)
Originally posted by @real-or-random in #1349 (comment)
