Extensive interface and operations tests for secp256k1_ec_pubkey_parse. (and some API paranoia improvements) #319

gmaxwell commented at 11:56 PM on September 27, 2015: contributor

This also makes use of optional valgrind instrumentation if -DVALGRIND is set.

This also moves secp256k1.c above secp256k1.h in tests.c or otherwise
 we get non-null macros on the public functions which may defeat some
 of the VERIFY checks.

gmaxwell commented at 11:58 PM on September 27, 2015: contributor

Perhaps foolishly I made the flags handling in secp256k1_ec_pubkey_serialize absolutely strict. Is this what we want to do? ... would we really ever want to add any more flags there? (hybrid? 0_o).

in src/secp256k1.c:None in 475880283f outdated

 161 | @@ -159,6 +162,11 @@ int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *o
 162 |      secp256k1_ge Q;
 163 |  
 164 |      (void)ctx;
 165 | +    VERIFY_CHECK(ctx != NULL);
 166 | +    ARG_CHECK(output != NULL);
 167 | +    ARG_CHECK(outputlen != NULL);
 168 | +    ARG_CHECK(pubkey != NULL);
 169 | +    ARG_CHECK((flags | SECP256K1_EC_COMPRESSED) == SECP256K1_EC_COMPRESSED);

sipa commented at 11:30 PM on September 29, 2015:

There is more advanced flag checking in #324. Can you leave it for now?

gmaxwell commented at 11:41 PM on September 29, 2015:

Sure. Do you mean leave all this checking out or just the flag part?

sipa commented at 1:24 AM on September 30, 2015:

Just the flag part.

gmaxwell commented at 4:39 AM on September 30, 2015: contributor

Okay, flags test gone.

in src/tests.c:None in 719f0c3c9d outdated

1973 | +        }
1974 | +    };
1975 | +#define SECP256K1_EC_PARSE_TEST_NINVALID (7)
1976 | +    const unsigned char invalid[SECP256K1_EC_PARSE_TEST_NINVALID][64] = {
1977 | +        {
1978 | +            /* x is third root of -8, y is abs(x^3+7); also on the curve for y^2 = x^3 + 9. */

sipa commented at 2:55 PM on September 30, 2015:

I don't think 'abs' has much meaning in a finite field...

sipa commented at 7:36 PM on September 30, 2015:

Do you mean sqrt?

in src/tests.c:None in e80959175b outdated

1800 | +            pubkeyc[0] = i;
1801 | +            /* What sign does this point have? */
1802 | +            ysign = (input[63] & 1) + 2;
1803 | +            /* For the current type (i) do we expect parsing to work? Handled all of compressed/uncompressed/hybrid. */
1804 | +            xpass = xvalid && (pubkeyclen == 33) && ((i & 254) == 2);
1805 | +            ypass = xvalid && yvalid && ((i & 4) == ((pubkeyclen == 65) << 2)) &&

sipa commented at 2:02 PM on October 13, 2015:

I'm confused about what ypass means, if it can also be true for 33-byte pubkeys?

sipa commented at 2:19 PM on October 13, 2015:

Got it. It means "when taking the current prefix byte + subsequence of input, we can parse and reserialize the result as uncompressed, and it will be identical to the input". Took me 10 minutes :)

in src/tests.c:None in e80959175b outdated

2075 | +        CHECK(ecount == 0);
2076 | +        CHECK(secp256k1_pubkey_load(ctx, &ge, &pubkey) == 0);
2077 | +        CHECK(ecount == 1);
2078 | +    }
2079 | +    /* 33 bytes claimed on otherwise valid input starting with 0x04, fail, zeroize output, no illegal arg error. */
2080 | +    memset(&pubkey, 0xfe, sizeof(pubkey));

sipa commented at 2:28 PM on October 13, 2015:

Seems it starts with 0xfe, not with 0x04?

sipa commented at 2:31 PM on October 13, 2015:

Nevermind, pubkeyc vs pubkey...

in src/tests.c:None in e80959175b outdated

2108 | +    CHECK(ecount == 1);
2109 | +    /* 66 bytes claimed, fail, zeroize output, no illegal arg error. */
2110 | +    memset(&pubkey, 0xfe, sizeof(pubkey));
2111 | +    ecount = 0;
2112 | +    VG_UNDEF(&pubkey, sizeof(pubkey));
2113 | +    CHECK(secp256k1_ec_pubkey_parse(ctx, &pubkey, pubkeyc, 66) == 0);

sipa commented at 2:35 PM on October 13, 2015:

That looks like an error. We know ec_pubkey_parse won't go look beyond the 65th byte, but it's a bit ugly that the correctness of the test code relies on that. Can you add a padding byte to pubkeyc?

sipa commented at 2:36 PM on October 13, 2015: contributor

ACK with tiny nits.

Add ARG_CHECKs to secp256k1_ec_pubkey_parse/secp256k1_ec_pubkey_serialize

This also makes secp256k1_ec_pubkey_parse's init of pubkey more unconditional.

ee2cb4007d

Extensive interface and operations tests for secp256k1_ec_pubkey_parse.

This also makes use of optional valgrind instrumentation if -DVALGRIND
 is set.

This also moves secp256k1.c above secp256k1.h in tests.c or otherwise
 we get non-null macros on the public functions which may defeat some
 of the VERIFY checks.

67f7da4087

gmaxwell commented at 2:00 AM on October 14, 2015: contributor

Nits should all be corrected now.

sipa merged this on Oct 14, 2015

sipa closed this on Oct 14, 2015

sipa referenced this in commit c98df263ed on Oct 14, 2015