tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID #1390

pull jonasnick wants to merge 4 commits into bitcoin-core:master from jonasnick:check-illegal changing 5 files +180 −501

jonasnick commented at 10:04 PM on July 28, 2023: contributor

Fixes #1167
jonasnick force-pushed on Jul 29, 2023

in src/tests.c:6631 in 7120fc25a3 outdated

6620 | +        {
6621 | +            int32_t ecount = 0;
6622 | +            secp256k1_context_set_illegal_callback(CTX, counting_illegal_callback_fn, &ecount);
6623 | +            CHECK(secp256k1_ec_pubkey_cmp(CTX, &pk_tmp, &pk_tmp) == 0);
6624 | +            CHECK(ecount == 2);
6625 | +            secp256k1_context_set_illegal_callback(CTX, NULL, NULL);

real-or-random commented at 12:01 PM on July 31, 2023:

Is it a bug that secp256k1_ec_pubkey_cmp calls the callback twice?

I mean, we never do this anywhere else, even if multiple arguments are illegal. In particular, ARG_CHECK makes the function return immediately on failure.

jonasnick commented at 12:53 PM on July 31, 2023:

It's intentional, see the comment in pubkey_cmp:

        /* If the public key is NULL or invalid, ec_pubkey_serialize will call
         * the illegal_callback and return 0. In that case we will serialize the
         * key as all zeros which is less than any valid public key. This
         * results in consistent comparisons even if NULL or invalid pubkeys are
         * involved and prevents edge cases such as sorting algorithms that use
         * this function and do not terminate as a result. */

In particular, returning 0 immediately in pubkey_cmp if parsing fails does not signal an error to the color, but that both public keys are identical.

real-or-random commented at 1:57 PM on July 31, 2023:

Oh, crazy. I still think we could get the best of both worlds and retain the current all-zeros behavior and at the same time call the callback only once. But that's better done in a separate PR then.

in src/modules/extrakeys/tests_impl.h:138 in 7120fc25a3 outdated

 147 | +    {
 148 | +        int32_t ecount = 0;
 149 | +        secp256k1_context_set_illegal_callback(CTX, counting_illegal_callback_fn, &ecount);
 150 | +        CHECK(secp256k1_xonly_pubkey_cmp(CTX, &pk1, &pk1) == 0);
 151 | +        CHECK(ecount == 2);
 152 | +        secp256k1_context_set_illegal_callback(CTX, NULL, NULL);

real-or-random commented at 12:02 PM on July 31, 2023:

(if you change it, also change it here)

in src/modules/ecdh/tests_impl.h:28 in 7120fc25a3 outdated

  25 | @@ -26,31 +26,19 @@ static int ecdh_hash_function_custom(unsigned char *output, const unsigned char
  26 |  
  27 |  static void test_ecdh_api(void) {
  28 |      /* Setup context that just counts errors */

real-or-random commented at 12:07 PM on July 31, 2023:

nit: Comment needs to be removed (or changed)

jonasnick commented at 1:20 PM on July 31, 2023:

fixed

in src/tests.c:5939 in 7120fc25a3 outdated
```
5915 | @@ -5962,142 +5916,99 @@ static void run_ec_pubkey_parse_test(void) {
5916 |          0xB8, 0x00
5917 |      };
5918 |      unsigned char sout[65];
5919 | -    unsigned char shortkey[2];
5920 | +    unsigned char shortkey[2] = { 0 };
```
real-or-random commented at 12:19 PM on July 31, 2023:

nit:

unsigned char shortkey[2] = { 0x03, 0x05 };

0x0500000000000000000000000000000000000000000000000000000000000000 is a valid x-coord with odd y-coord
jonasnick commented at 12:43 PM on July 31, 2023:

I'm not sure how this would improves the tests.

real-or-random commented at 1:52 PM on July 31, 2023:

Yeah sorry, I didn't think it through entirely.
real-or-random commented at 12:20 PM on July 31, 2023: contributor
utACK mod nits

This makes the test code much cleaner.

I suggest (this PR or followup)
- changing secp256k1_ec_pubkey_cmp to call a callback at most once (see review)
- adding a macro CHECK_ERROR for the error_callback (which should probably involve renaming counting_illegal_callback to just counting_callback -- the callback doesn't care what it's used for)
real-or-random added the label assurance on Jul 31, 2023
real-or-random added the label refactor/smell on Jul 31, 2023
jonasnick force-pushed on Jul 31, 2023
jonasnick commented at 1:20 PM on July 31, 2023: contributor
adding a macro CHECK_ERROR for the error_callback (which should probably involve renaming counting_illegal_callback to just counting_callback -- the callback doesn't care what it's used for)
done
real-or-random commented at 2:29 PM on July 31, 2023: contributor
Ah, my intention behind the non-VOID macros is that they can also be used for functions that return pointers (because NULL == 0 as guaranteed by the standard), e.g., as in the existing line:
```
CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_preallocated_clone(STATIC_CTX, my_static_ctx));
```
That means we could also have CHECK_ERROR and use it. Do these fixups make sense? https://github.com/real-or-random/secp256k1/commits/check-illegal I also tried to clarify the comments to say that NULL pointers are covered.

edit: Alternatively, if you think that == NULL is more explicit, we should probably do this everywhere.
real-or-random commented at 2:37 PM on July 31, 2023: contributor

The set_illegal_callback calls in ec_pubkey_parse_pointtest and run_eckey_edge_case_test should also be removed.
jonasnick force-pushed on Jul 31, 2023
real-or-random approved
real-or-random commented at 9:21 AM on August 1, 2023: contributor

utACK fb5eb75a89598394f615eb55f0757e7c35e420ba
siv2r commented at 4:41 AM on August 29, 2023: contributor

The error_callback and illegal_callback are being set to NULL at the end of test_keypair fn (in extrakeys/tests_impl.h). Shouldn't we remove these too?

Github doesn't allow me to comment on these lines directly. So, here's the link: https://github.com/bitcoin-core/secp256k1/pull/1390/files#diff-07d311190e50e344466bc33ed56c60d2a15f4e8221bfc97792e1cd441b8f3e19L443-L444
siv2r commented at 4:44 AM on August 29, 2023: contributor

ACK fb5eb75, the tests locally pass on my machine (for all four commits).
real-or-random commented at 9:48 AM on August 29, 2023: contributor

needs rebase :)
tests: remove unnecessary set_illegal_callback 875b0ada25

tests: remove unnecessary test in run_ec_pubkey_parse_test

This test tested whether setting the callback works correctly which should be
tested in the context tests.

a1d52e3e12

jonasnick force-pushed on Sep 4, 2023
jonasnick commented at 12:53 PM on September 4, 2023: contributor

rebased

tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID

This commit also explicitly initializes shortpubkey. For some reason, removing
surrounding, unrelated lines results in gcc warnings when configured with
--enable-ctime-tests=no --with-valgrind=no.

f8d7ea68df

tests: add CHECK_ERROR_VOID and use it in scratch tests 70303643cf
jonasnick force-pushed on Sep 4, 2023
jonasnick commented at 4:20 PM on September 4, 2023: contributor

@siv2r

The error_callback and illegal_callback are being set to NULL at the end of test_keypair fn (in extrakeys/tests_impl.h).

Good catch, fixed.
real-or-random approved
real-or-random commented at 4:27 PM on September 4, 2023: contributor

reACK 70303643cf42d18acbf1c020480c6bb23072dbd9
siv2r commented at 4:55 PM on September 4, 2023: contributor

reACK 7030364 (tests pass locally)
real-or-random merged this on Sep 4, 2023
real-or-random closed this on Sep 4, 2023

Contributors

jonasnick

real-or-random

siv2r

Labels

assurance refactor/smell

Linked (view graph)

#1167 Setting counting_illegal_callback may hide failing tests #1727 docs: Clarify that callback can be called more than once