Fix review comments by Sipa

Author: elichai

examples/ecdh.c

@@ -8,9 +8,10 @@
 #include <assert.h>
 #include <string.h>
 
+#include <secp256k1.h>
+#include <secp256k1_ecdh.h>
+
 #include "random.h"
-#include "secp256k1.h"
-#include "secp256k1_ecdh.h"
 
 
 int main(void) {
@@ -21,13 +22,14 @@ int main(void) {
     unsigned char shared_secret1[32];
     unsigned char shared_secret2[32];
     unsigned char randomize[32];
+    int return_val;
     size_t len;
     secp256k1_pubkey pubkey1;
     secp256k1_pubkey pubkey2;
 
-    /* The docs in secp256k1.h above the `secp256k1_ec_pubkey_create` function
-     * say: "pointer to a context object, initialized for signing" which is why
-     * we create a context for signing with the SECP256K1_CONTEXT_SIGN flag.
+    /* The specification in secp256k1.h states that `secp256k1_ec_pubkey_create`
+     * needs a context object initialized for signing, which is why we create
+     * a context with the SECP256K1_CONTEXT_SIGN flag.
      * (The docs for `secp256k1_ecdh` don't require any special context, just
      * some initialized context) */
     secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
@@ -38,7 +40,8 @@ int main(void) {
     /* Randomizing the context is recommended to protect against side-channel
      * leakage See `secp256k1_context_randomize` in secp256k1.h for more
      * information about it. This should never fail. */
-    assert(secp256k1_context_randomize(ctx, randomize));
+    return_val = secp256k1_context_randomize(ctx, randomize);
+    assert(return_val);
 
     /*** Key Generation ***/
 
@@ -56,32 +59,40 @@ int main(void) {
     }
 
     /* Public key creation using a valid context with a verified secret key should never fail */
-    assert(secp256k1_ec_pubkey_create(ctx, &pubkey1, seckey1));
-    assert(secp256k1_ec_pubkey_create(ctx, &pubkey2, seckey2));
+    return_val = secp256k1_ec_pubkey_create(ctx, &pubkey1, seckey1);
+    assert(return_val);
+    return_val = secp256k1_ec_pubkey_create(ctx, &pubkey2, seckey2);
+    assert(return_val);
 
     /* Serialize pubkey1 in a compressed form (33 bytes), should always return 1 */
     len = sizeof(compressed_pubkey1);
-    assert(secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey1, &len, &pubkey1, SECP256K1_EC_COMPRESSED));
+    return_val = secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey1, &len, &pubkey1, SECP256K1_EC_COMPRESSED);
+    assert(return_val);
     /* Should be the same size as the size of the output, because we passed a 33 bytes array. */
     assert(len == sizeof(compressed_pubkey1));
 
     /* Serialize pubkey2 in a compressed form (33 bytes) */
     len = sizeof(compressed_pubkey2);
-    secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey2, &len, &pubkey2, SECP256K1_EC_COMPRESSED);
+    return_val = secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey2, &len, &pubkey2, SECP256K1_EC_COMPRESSED);
+    assert(return_val);
+    /* Should be the same size as the size of the output, because we passed a 33 bytes array. */
     assert(len == sizeof(compressed_pubkey2));
 
     /*** Creating the shared secret ***/
 
     /* Perform ECDH with seckey1 and pubkey2. Should never fail with a verified
      * seckey and valid pubkey */
-    assert(secp256k1_ecdh(ctx, shared_secret1, &pubkey2, seckey1, NULL, NULL));
+    return_val = secp256k1_ecdh(ctx, shared_secret1, &pubkey2, seckey1, NULL, NULL);
+    assert(return_val);
 
     /* Perform ECDH with seckey2 and pubkey1. Should never fail with a verified
      * seckey and valid pubkey */
-    assert(secp256k1_ecdh(ctx, shared_secret2, &pubkey1, seckey2, NULL, NULL));
+    return_val = secp256k1_ecdh(ctx, shared_secret2, &pubkey1, seckey2, NULL, NULL);
+    assert(return_val);
 
     /* Both parties should end up with the same shared secret */
-    assert(memcmp(shared_secret1, shared_secret2, sizeof(shared_secret1)) == 0);
+    return_val = memcmp(shared_secret1, shared_secret2, sizeof(shared_secret1));
+    assert(return_val == 0);
 
     printf("Secret Key1: ");
     print_hex(seckey1, sizeof(seckey1));
@@ -97,10 +108,10 @@ int main(void) {
     /* This will clear everything from the context and free the memory */
     secp256k1_context_destroy(ctx);
 
-    /* It's best practice to try to remove secrets from memory after using them.
-     * This is done because some bugs can allow an attacker leak memory, for
-     * example through out of bounds array access (see Heartbleed for example).
-     * Hence, we overwrite the secret key buffer with zeros.
+    /* It's best practice to try to clear secrets from memory after using them.
+     * This is done because some bugs can allow an attacker to leak memory, for
+     * example through "out of bounds" array access (see Heartbleed), Or the OS
+     * swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
      *
      * TODO: Prevent these writes from being optimized out, as any good compiler
      * will remove any writes that aren't used. */

examples/ecdsa.c

@@ -8,8 +8,9 @@
 #include <assert.h>
 #include <string.h>
 
+#include <secp256k1.h>
+
 #include "random.h"
-#include "secp256k1.h"
 
 
 
@@ -21,12 +22,12 @@ int main(void) {
     unsigned char serialized_signature[64];
     size_t len;
     int is_signature_valid;
+    int return_val;
     secp256k1_pubkey pubkey;
     secp256k1_ecdsa_signature sig;
-    /* The docs in secp256k1.h above the `secp256k1_ec_pubkey_create` function
-     * say: "pointer to a context object, initialized for signing" And the docs
-     * above the `secp256k1_ecdsa_verify` function say: "a secp256k1 context
-     * object, initialized for verification" which is why we create a context
+    /* The specification in secp256k1.h states that `secp256k1_ec_pubkey_create` needs
+     * a context object initialized for signing and `secp256k1_ecdsa_verify` needs
+     * a context initialized for verification, which is why we create a context
      * for both signing and verification with the SECP256K1_CONTEXT_SIGN and
      * SECP256K1_CONTEXT_VERIFY flags. */
     secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
@@ -37,7 +38,8 @@ int main(void) {
     /* Randomizing the context is recommended to protect against side-channel
      * leakage See `secp256k1_context_randomize` in secp256k1.h for more
      * information about it. This should never fail. */
-    assert(secp256k1_context_randomize(ctx, randomize));
+    return_val = secp256k1_context_randomize(ctx, randomize);
+    assert(return_val);
 
     /*** Key Generation ***/
 
@@ -55,11 +57,13 @@ int main(void) {
     }
 
     /* Public key creation using a valid context with a verified secret key should never fail */
-    assert(secp256k1_ec_pubkey_create(ctx, &pubkey, seckey));
+    return_val = secp256k1_ec_pubkey_create(ctx, &pubkey, seckey);
+    assert(return_val);
 
     /* Serialize the pubkey in a compressed form(33 bytes). Should always return 1. */
     len = sizeof(compressed_pubkey);
-    assert(secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey, &len, &pubkey, SECP256K1_EC_COMPRESSED));
+    return_val = secp256k1_ec_pubkey_serialize(ctx, compressed_pubkey, &len, &pubkey, SECP256K1_EC_COMPRESSED);
+    assert(return_val);
     /* Should be the same size as the size of the output, because we passed a 33 bytes array. */
     assert(len == sizeof(compressed_pubkey));
 
@@ -70,11 +74,13 @@ int main(void) {
     * `noncefp` and `ndata` allows you to pass a custom nonce function, passing
     * `NULL` will use the RFC-6979 safe default. Signing with a valid context,
     * verified secret key and the default nonce function should never fail. */
-    assert(secp256k1_ecdsa_sign(ctx, &sig, msg_hash, seckey, NULL, NULL));
+    return_val = secp256k1_ecdsa_sign(ctx, &sig, msg_hash, seckey, NULL, NULL);
+    assert(return_val);
 
     /* Serialize the signature in a compact form. Should always return 1
      * according to the documentation in secp256k1.h. */
-    assert(secp256k1_ecdsa_signature_serialize_compact(ctx, serialized_signature, &sig));
+    return_val = secp256k1_ecdsa_signature_serialize_compact(ctx, serialized_signature, &sig);
+    assert(return_val);
 
 
     /*** Verification ***/
@@ -108,10 +114,10 @@ int main(void) {
     /* This will clear everything from the context and free the memory */
     secp256k1_context_destroy(ctx);
 
-    /* It's best practice to try to remove secrets from memory after using them.
-     * This is done because some bugs can allow an attacker leak memory, for
-     * example through out of bounds array access (see Heartbleed for example).
-     * Hence, we overwrite the secret key buffer with zeros.
+    /* It's best practice to try to clear secrets from memory after using them.
+     * This is done because some bugs can allow an attacker to leak memory, for
+     * example through "out of bounds" array access (see Heartbleed), Or the OS
+     * swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
      *
      * TODO: Prevent these writes from being optimized out, as any good compiler
      * will remove any writes that aren't used. */

examples/random.h

@@ -7,6 +7,7 @@
 /*
  * This file is an attempt at collecting best practice methods for obtaining randomness with different operating systems.
  * It may be out-of-date. Consult the documentation of the operating system before considering to use the methods below.
+ *
  * Platform randomness sources:
  * Linux   -> `getrandom(2)`(`sys/random.h`), if not available `/dev/urandom` should be used. http://man7.org/linux/man-pages/man2/getrandom.2.html, https://linux.die.net/man/4/urandom
  * macOS   -> `getentropy(2)`(`sys/random.h`), if not available `/dev/urandom` should be used. https://www.unix.com/man-page/mojave/2/getentropy, https://opensource.apple.com/source/xnu/xnu-517.12.7/bsd/man/man4/random.4.auto.html

examples/schnorr.c

@@ -8,11 +8,11 @@
 #include <assert.h>
 #include <string.h>
 
-#include "random.h"
-#include "secp256k1.h"
-#include "secp256k1_extrakeys.h"
-#include "secp256k1_schnorrsig.h"
+#include <secp256k1.h>
+#include <secp256k1_extrakeys.h>
+#include <secp256k1_schnorrsig.h>
 
+#include "random.h"
 
 int main(void) {
     unsigned char msg_hash[32] = {0}; /* This should be a hash of the message. */
@@ -22,14 +22,14 @@ int main(void) {
     unsigned char serialized_pubkey[32];
     unsigned char signature[64];
     int is_signature_valid;
+    int return_val;
     secp256k1_xonly_pubkey pubkey;
     secp256k1_keypair keypair;
-    /* The docs in secp256k1_extrakeys.h above the `secp256k1_keypair_create`
-     * function say: "pointer to a context object, initialized for signing" And
-     * the docs above the `secp256k1_schnorrsig_verify` function say: "a
-     * secp256k1 context object, initialized for verification" which is why we
-     * create a context for both signing and verification with the
-     * SECP256K1_CONTEXT_SIGN and SECP256K1_CONTEXT_VERIFY flags. */
+    /* The specification in secp256k1_extrakeys.h states that `secp256k1_keypair_create`
+     * needs a context object initialized for signing. And in secp256k1_schnorrsig.h
+     * they state that `secp256k1_schnorrsig_verify` needs a context initialized for
+     * verification, which is why we create a context for both signing and verification
+     * with the SECP256K1_CONTEXT_SIGN and SECP256K1_CONTEXT_VERIFY flags. */
     secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
     if (!fill_random(randomize, sizeof(randomize))) {
         printf("Failed to generate randomness\n");
@@ -38,8 +38,8 @@ int main(void) {
     /* Randomizing the context is recommended to protect against side-channel
      * leakage See `secp256k1_context_randomize` in secp256k1.h for more
      * information about it. This should never fail. */
-    assert(secp256k1_context_randomize(ctx, randomize));
-
+    return_val = secp256k1_context_randomize(ctx, randomize);
+    assert(return_val);
     /*** Key Generation ***/
 
     /* If the secret key is zero or out of range (bigger than secp256k1's
@@ -61,10 +61,12 @@ int main(void) {
      * `pk_parity` as we don't care about the parity of the key, only advanced
      * users might care about the parity. This should never fail with a valid
      * context and public key. */
-    assert(secp256k1_keypair_xonly_pub(ctx, &pubkey, NULL, &keypair));
+    return_val = secp256k1_keypair_xonly_pub(ctx, &pubkey, NULL, &keypair);
+    assert(return_val);
 
     /* Serialize the public key. Should always return 1 for a valid public key. */
-    assert(secp256k1_xonly_pubkey_serialize(ctx, serialized_pubkey, &pubkey));
+    return_val = secp256k1_xonly_pubkey_serialize(ctx, serialized_pubkey, &pubkey);
+    assert(return_val);
 
     /*** Signing ***/
 
@@ -80,7 +82,8 @@ int main(void) {
     * improve security against side-channel attacks. Signing with a valid
     * context, verified keypair and the default nonce function should never
     * fail. */
-    assert(secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand));
+    return_val = secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand);
+    assert(return_val);
 
     /*** Verification ***/
 
@@ -106,10 +109,10 @@ int main(void) {
     /* This will clear everything from the context and free the memory */
     secp256k1_context_destroy(ctx);
 
-    /* It's best practice to try to remove secrets from memory after using them.
-     * This is done because some bugs can allow an attacker leak memory, for
-     * example through out of bounds array access (see Heartbleed for example).
-     * Hence, we overwrite the secret key buffer with zeros.
+    /* It's best practice to try to clear secrets from memory after using them.
+     * This is done because some bugs can allow an attacker to leak memory, for
+     * example through "out of bounds" array access (see Heartbleed), Or the OS
+     * swapping them to disk. Hence, we overwrite the secret key buffer with zeros.
      *
      * TODO: Prevent these writes from being optimized out, as any good compiler
      * will remove any writes that aren't used. */