add fuzz tests about field operations. #1385
Conversation
| selected_fuzz_function = &fuzz_field_storage_cmov; | ||
| } else { | ||
| fprintf(stderr, "Unknown fuzz test selected using FUZZ environment variable: %s\n", test_name); | ||
| assert(false); |
There was a problem hiding this comment.
false doesn't exist as a keyword in C89 (which the libsecp256k1 source code tries to use with the least amount of extensions possible). You can use assert(0);.
| if (secp256k1_fe_normalizes_to_zero(&a)) { | ||
| CHECK(secp256k1_fe_normalizes_to_zero(&r1)); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Coding style nit: } else { can go on one line.
| if (secp256k1_scalar_is_zero(&a)) { | ||
| CHECK(secp256k1_scalar_is_zero(&r1)); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Style nit: } else { on one line.
| if (secp256k1_scalar_is_zero(&a)) { | ||
| CHECK(secp256k1_scalar_is_zero(&r1)); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Style nit: } else { on one line.
| int bit, r1, r2; | ||
| secp256k1_scalar a; | ||
| secp256k1_scalar_set_b32(&a, data, NULL); | ||
| bit = 1 + (size % 15); |
There was a problem hiding this comment.
I don't recommend using the fuzzer input's length as input itself; I don't think it's really designed to covney much information.
Suggestion: bit = 1 + (data[32] % 15), and if (size >= 33) {.
| /*** Scalar Operation ***/ | ||
| /* Test commutativity of scalar addition */ | ||
| static void fuzz_scalar_add_commutativty(const uint8_t *data, size_t size) { | ||
| if (size > 63) { |
There was a problem hiding this comment.
Tiny nit, but I think it's a bit more readable to use if (size >= 64) { (here and elsewhere), as the 64 is the actual size we need.
| static void fuzz_field_equality(const uint8_t *data, size_t size) { | ||
| if (size > 31) { | ||
| secp256k1_fe fe; | ||
| secp256k1_fe_set_b32_mod(&fe, data); |
There was a problem hiding this comment.
This will only construct very "benign" field elements, which are perhaps not the most likely ones to trigger issues.
Going beyond this is non-trivial though. You'll probably want to write a helper function to construct a field element with specified representation directly (so the fe.d[0..4] elements, in particular) and then bail out if it turns out to be invalid. Then you'll need to set magnitude/normalization appropriately as well.
This will involve a number of changes, and you'll want to read up on the field element representation (see field_5x52.h, let's ignore the 26-bit field for now). It'll also mean some refactoring of secp256k1_fe_impl_verify.
|
Superseded by #1407 |
Thanks for the previous review.
This PR:
Could you give me some suggestions?