Better error case handling for pubkey_create & pubkey_serialize, more tests.

Makes secp256k1_ec_pubkey_serialize set the length to zero on failure,
 also makes secp256k1_ec_pubkey_create set the pubkey to zeros when
 the key argument is NULL.

Also adds many additional ARGCHECK tests.

diff --git a/src/secp256k1.c b/src/secp256k1.c
index a4eb34504f9a9e88e3a319f843adf9f58a9b4ae4..e06f7d942e5f9c887a53ff77106b62ff0790603a 100644 (file)
--- a/src/secp256k1.c
+++ b/src/secp256k1.c
@@ -167,15 +167,25 @@ int secp256k1_ec_pubkey_parse(const secp256k1_context* ctx, secp256k1_pubkey* pu
 
 int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *output, size_t *outputlen, const secp256k1_pubkey* pubkey, unsigned int flags) {
     secp256k1_ge Q;
+    size_t len;
+    int ret = 0;
 
     (void)ctx;
     VERIFY_CHECK(ctx != NULL);
-    ARG_CHECK(output != NULL);
     ARG_CHECK(outputlen != NULL);
+    len = *outputlen;
+    *outputlen = 0;
+    ARG_CHECK(output != NULL);
+    memset(output, 0, len);
     ARG_CHECK(pubkey != NULL);
     ARG_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_COMPRESSION);
-    return (secp256k1_pubkey_load(ctx, &Q, pubkey) &&
-            secp256k1_eckey_pubkey_serialize(&Q, output, outputlen, flags & SECP256K1_FLAGS_BIT_COMPRESSION));
+    if (secp256k1_pubkey_load(ctx, &Q, pubkey)) {
+        ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, flags & SECP256K1_FLAGS_BIT_COMPRESSION);
+        if (ret) {
+            *outputlen = len;
+        }
+    }
+    return ret;
 }
 
 static void secp256k1_ecdsa_signature_load(const secp256k1_context* ctx, secp256k1_scalar* r, secp256k1_scalar* s, const secp256k1_ecdsa_signature* sig) {
@@ -402,13 +412,13 @@ int secp256k1_ec_pubkey_create(const secp256k1_context* ctx, secp256k1_pubkey *p
     int overflow;
     int ret = 0;
     VERIFY_CHECK(ctx != NULL);
-    ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
     ARG_CHECK(pubkey != NULL);
+    memset(pubkey, 0, sizeof(*pubkey));
+    ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
     ARG_CHECK(seckey != NULL);
 
     secp256k1_scalar_set_b32(&sec, seckey, &overflow);
     ret = (!overflow) & (!secp256k1_scalar_is_zero(&sec));
-    memset(pubkey, 0, sizeof(*pubkey));
     if (ret) {
         secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pj, &sec);
         secp256k1_ge_set_gej(&p, &pj);

diff --git a/src/tests.c b/src/tests.c
index 1e63659dbf3ccf73d20988c11808afc7b82c51e2..115a832023417bccdef9695ddf9b62a0e75bfbf8 100644 (file)
--- a/src/tests.c
+++ b/src/tests.c
@@ -232,6 +232,8 @@ void run_context_tests(void) {
     secp256k1_context_destroy(sign);
     secp256k1_context_destroy(vrfy);
     secp256k1_context_destroy(both);
+    /* Defined as no-op. */
+    secp256k1_context_destroy(NULL);
 }
 
 /***** HASH TESTS *****/
@@ -2166,9 +2168,11 @@ void run_ec_pubkey_parse_test(void) {
         0xA8, 0xFD, 0x17, 0xB4, 0x48, 0xA6, 0x85, 0x54, 0x19, 0x9C, 0x47, 0xD0, 0x8F, 0xFB, 0x10, 0xD4,
         0xB8, 0x00
     };
+    unsigned char sout[65];
     unsigned char shortkey[2];
     secp256k1_ge ge;
     secp256k1_pubkey pubkey;
+    size_t len;
     int32_t i;
     int32_t ecount;
     int32_t ecount2;
@@ -2265,6 +2269,30 @@ void run_ec_pubkey_parse_test(void) {
     VG_CHECK(&ge.infinity, sizeof(ge.infinity));
     ge_equals_ge(&secp256k1_ge_const_g, &ge);
     CHECK(ecount == 0);
+    /* secp256k1_ec_pubkey_serialize illegal args. */
+    ecount = 0;
+    len = 65;
+    CHECK(secp256k1_ec_pubkey_serialize(ctx, NULL, &len, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 0);
+    CHECK(ecount == 1);
+    CHECK(len == 0);
+    CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, NULL, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 0);
+    CHECK(ecount == 2);
+    len = 65;
+    VG_UNDEF(sout, 65);
+    CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, NULL, SECP256K1_EC_UNCOMPRESSED) == 0);
+    VG_CHECK(sout, 65);
+    CHECK(ecount == 3);
+    CHECK(len == 0);
+    len = 65;
+    CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, &pubkey, ~0) == 0);
+    CHECK(ecount == 4);
+    CHECK(len == 0);
+    len = 65;
+    VG_UNDEF(sout, 65);
+    CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 1);
+    VG_CHECK(sout, 65);
+    CHECK(ecount == 4);
+    CHECK(len == 65);
     /* Multiple illegal args. Should still set arg error only once. */
     ecount = 0;
     ecount2 = 11;
@@ -2453,7 +2481,7 @@ void run_eckey_edge_case_test(void) {
     memset(&pubkey, 1, sizeof(pubkey));
     CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, NULL) == 0);
     CHECK(ecount == 2);
-    CHECK(memcmp(&pubkey, zeros, sizeof(secp256k1_pubkey)) > 0);
+    CHECK(memcmp(&pubkey, zeros, sizeof(secp256k1_pubkey)) == 0);
     secp256k1_context_set_illegal_callback(ctx, NULL, NULL);
 }
 
@@ -2648,6 +2676,7 @@ void test_ecdsa_end_to_end(void) {
     CHECK(secp256k1_ecdsa_signature_normalize(ctx, NULL, &signature[5]));
     CHECK(secp256k1_ecdsa_signature_normalize(ctx, &signature[5], &signature[5]));
     CHECK(!secp256k1_ecdsa_signature_normalize(ctx, NULL, &signature[5]));
+    CHECK(!secp256k1_ecdsa_signature_normalize(ctx, &signature[5], &signature[5]));
     CHECK(secp256k1_ecdsa_verify(ctx, &signature[5], message, &pubkey) == 1);
     secp256k1_scalar_negate(&s, &s);
     secp256k1_ecdsa_signature_save(&signature[5], &r, &s);
@@ -3286,17 +3315,46 @@ void test_ecdsa_edge_cases(void) {
         CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, NULL) == 0);
         CHECK(ecount == 6);
         CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, &pubkey) == 1);
+        CHECK(ecount == 6);
+        CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, NULL) == 0);
+        CHECK(ecount == 7);
+        /* That pubkeyload fails via an ARGCHECK is a little odd but makes sense because pubkeys are an opaque data type. */
+        CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, &pubkey) == 0);
+        CHECK(ecount == 8);
         siglen = 72;
         CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, NULL, &siglen, &sig) == 0);
-        CHECK(ecount == 7);
+        CHECK(ecount == 9);
         CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, NULL, &sig) == 0);
-        CHECK(ecount == 8);
+        CHECK(ecount == 10);
         CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, NULL) == 0);
-        CHECK(ecount == 9);
+        CHECK(ecount == 11);
         CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, &sig) == 1);
+        CHECK(ecount == 11);
+        CHECK(secp256k1_ecdsa_signature_parse_der(ctx, NULL, signature, siglen) == 0);
+        CHECK(ecount == 12);
+        CHECK(secp256k1_ecdsa_signature_parse_der(ctx, &sig, NULL, siglen) == 0);
+        CHECK(ecount == 13);
+        CHECK(secp256k1_ecdsa_signature_parse_der(ctx, &sig, signature, siglen) == 1);
+        CHECK(ecount == 13);
         siglen = 10;
+        /* Too little room for a signature does not fail via ARGCHECK. */
         CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, &sig) == 0);
-        CHECK(ecount == 9);
+        CHECK(ecount == 13);
+        ecount = 0;
+        CHECK(secp256k1_ecdsa_signature_normalize(ctx, NULL, NULL) == 0);
+        CHECK(ecount == 1);
+        CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, NULL, &sig) == 0);
+        CHECK(ecount == 2);
+        CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, signature, NULL) == 0);
+        CHECK(ecount == 3);
+        CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, signature, &sig) == 1);
+        CHECK(ecount == 3);
+        CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, NULL, signature) == 0);
+        CHECK(ecount == 4);
+        CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, NULL) == 0);
+        CHECK(ecount == 5);
+        CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, signature) == 1);
+        CHECK(ecount == 5);
         secp256k1_context_set_illegal_callback(ctx, NULL, NULL);
     }