Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"sizeof(secp256k1_ge_storage) != 64" path not tested #1352

Closed
real-or-random opened this issue Jun 18, 2023 · 2 comments · Fixed by #1480
Closed

"sizeof(secp256k1_ge_storage) != 64" path not tested #1352

real-or-random opened this issue Jun 18, 2023 · 2 comments · Fixed by #1480

Comments

@real-or-random
Copy link
Contributor

real-or-random commented Jun 18, 2023

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_storage and secp256k1_fe_storage to 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)

@real-or-random
Copy link
Contributor Author

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.

A friend pointed out to me that, of course, the C ABI is pretty fixed on every platform, so that compilers don't have a lot of leeway here anyway.

@sipa
Copy link
Contributor

sipa commented Jan 7, 2024

Sounds good. Let's just make the assumption that the secp256k1_pubkey type is exactly 64 bytes explicit, and drop the branches.

real-or-random added a commit that referenced this issue Jan 9, 2024
…ode 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 #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@sipa @real-or-random and others