tests: Fix test whose result is implementation-defined #1054

real-or-random commented at 6:32 PM on December 23, 2021: contributor

A compiler may add struct padding and fe_cmov is not guaranteed to preserve it.

On the way, we restore the name of the function. It was mistakenly renamed in 6173839c90553385171d560be8a17cbe167e3bef using "search and replace".

in src/tests.c:2456 in 3801e33d40 outdated

2458 | -    t.normalized = a->normalized;
2459 | -#endif
2460 | -    return secp256k1_memcmp_var(a, &t, sizeof(secp256k1_fe));
2461 | +int fe_memcmp(const secp256k1_fe *a, const secp256k1_fe *b) {
2462 | +    /* Compare only the struct member that holds the limbs
2463 | +       (there may be others in VERIFY mode). */

robot-dreams commented at 7:01 PM on December 23, 2021:

Nit: Clarify that only the limbs matter this comparison?

    /* Compare only the struct member that holds the limbs
       (there may be others in VERIFY mode, but this function
       should ignore them). */

robot-dreams commented at 7:02 PM on December 23, 2021: contributor

ACK 3801e33d400475c2fdaef1280cf8ed820505a45a assuming my understanding is correct:

fe_memcmp should only consider limbs, and ignore the magnitude and normalized fields (both before and after this change)
Before this change, secp256k1_memcmp_var could have returned nonzero even though all limbs are equal, in the case where a and t differ in uninitialized padding bytes

real-or-random commented at 7:05 PM on December 23, 2021: contributor

fe_memcmp should only consider limbs, and ignore the magnitude and normalized fields (both before and after this change)

Well I guess it wouldn't hurt to compare the VERIFY fields too...

robot-dreams commented at 7:05 PM on December 23, 2021: contributor

By the way, just curious, which compiler/architecture did you observe (or do you expect) to add padding?

real-or-random commented at 7:08 PM on December 23, 2021: contributor

By the way, just curious, which compiler/architecture did you observe (or do you expect) to add padding?

I don't know, my judgement is based on the C standard.

tests: Fix test whose result is implementation-defined

A compiler may add struct padding and fe_cmov is not guaranteed to
preserve it.

On the way, we improve the identity check such that it covers the
VERIFY struct members.

3d7cbafb5f

real-or-random force-pushed on Dec 23, 2021

real-or-random commented at 7:20 PM on December 23, 2021: contributor

Forced-push, now checks also equality of the VERIFY members.

robot-dreams commented at 7:25 PM on December 23, 2021: contributor

ACK 3d7cbafb5fd7f152fc47dc907af5df03150accc0

sipa commented at 7:26 PM on December 23, 2021: contributor

utACK 3d7cbafb5fd7f152fc47dc907af5df03150accc0

in src/tests.c:2462 in 3d7cbafb5f

2462 | +    ret &= (a->magnitude == b->magnitude);
2463 | +    ret &= (a->normalized == b->normalized);
2464 |  #endif
2465 | -    return secp256k1_memcmp_var(a, &t, sizeof(secp256k1_fe));
2466 | +    /* Compare the struct member that holds the limbs. */
2467 | +    ret &= (secp256k1_memcmp_var(a->n, b->n, sizeof(a->n)) == 0);

real-or-random commented at 7:30 PM on December 23, 2021:

In case anyone is wondering: I verified that sizeof(a->n) == 40 ( == 5 * 64 / 8 == 10 * 32 / 8)

sipa commented at 7:37 PM on December 23, 2021: contributor

By the way, just curious, which compiler/architecture did you observe (or do you expect) to add padding?

In a hypothetical system where int is larger than 64 bits, this could happen. I'm not sure that can be done while complying with the C89 (or later) standard, though, as it puts some restrictions on the sizes of integer types.

real-or-random commented at 7:51 PM on December 23, 2021: contributor

I know this is getting slightly off-topic but this is maybe educational.

In a hypothetical system where int is larger than 64 bits, this could happen.

Yeah, but not only there. I think C only requires alignment to be at least the size. But it may be larger. For example, there may be a hypothetical system where the compiler decides it's a good idea to add some padding because then the int can be accessed more quickly.

In practice though, struct padding is pretty much restricted by calling conventions (even though I'm not sure whether this argument would apply here -- maybe the compiler can leverage that this struct is not visible from the outside).

I'm not sure that can be done while complying with the C89 (or later) standard, though, as it puts some restrictions on the sizes of integer types.

C only specifies minimum ranges but no maximum value ranges. See https://en.wikipedia.org/wiki/C_data_types#Main_types for the ranges expressed in bits. (C also requires that the order makes sense signed char <= int <= long <= long long, similar for unsigned, and that signed and unsigned variant have a related range.)

real-or-random merged this on Dec 25, 2021

real-or-random closed this on Dec 25, 2021

jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick

real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022

dhruv referenced this in commit 6f7e395acc on Jan 26, 2022

hebasto referenced this in commit 065b6dbf9d on Jan 30, 2022

dhruv referenced this in commit 139d4b881e on Feb 1, 2022

fanquake referenced this in commit 8f8631d826 on Mar 17, 2022

fanquake referenced this in commit 4bb1d7e62a on Mar 17, 2022

fanquake referenced this in commit 465d05253a on Mar 30, 2022

stratospher referenced this in commit 4d5afc9767 on Apr 3, 2022

stratospher referenced this in commit 5b18493cfa on Apr 3, 2022

fanquake referenced this in commit afb7a6fe06 on Apr 6, 2022

gwillen referenced this in commit 35d6112a72 on May 25, 2022

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

janus referenced this in commit 3a0652a777 on Aug 4, 2022

str4d referenced this in commit 522190d5c3 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023