Skip to content

Commit

Permalink
Eliminate multiple-returns from secp256k1.c.
Browse files Browse the repository at this point in the history 
Goto, multiple returns, continue, and/or multiple breaks in a
 loop are often used to build complex or non-local control
 flow in software.

(They're all basically the same thing, and anyone axiomatically
 opposing goto and not the rest is probably cargo-culting from
 the title of Dijkstra's essay without thinking hard about it.)

Personally, I think the current use of these constructs in the
 code base is fine: no where are we using them to create control-
 flow that couldn't easily be described in plain English, which
 is hard to read or reason about, or which looks like a trap for
 future developers.

Some, however, prefer a more rules based approach to software
 quality.  In particular, MISRA forbids all of these constructs,
 and for good experience based reasons.  Rules also have the
 benefit of being machine checkable and surviving individual
 developers.

(To be fair-- MISRA also has a process for accommodating code that
 breaks the rules for good reason).

I think that in general we should also try to satisfy the rules-
 based measures of software quality, except where there is an
 objective reason not do: a measurable performance difference,
 logic that turns to spaghetti, etc.

Changing out all the multiple returns in secp256k1.c appears to
 be basically neutral:  Some parts become slightly less clear,
 some parts slightly more.
  • Loading branch information
@gmaxwell
gmaxwell committed Mar 8, 2015
1 parent 354ffa3 commit 0065a8f
Showing 1 changed file with 109 additions and 103 deletions.
212 changes: 109 additions & 103 deletions src/secp256k1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,24 +37,30 @@ int secp256k1_ecdsa_verify(const unsigned char *msg32, const unsigned char *sig,
secp256k1_ge_t q;
secp256k1_ecdsa_sig_t s;
secp256k1_scalar_t m;
int ret = -3;
DEBUG_CHECK(secp256k1_ecmult_consts != NULL);
DEBUG_CHECK(msg32 != NULL);
DEBUG_CHECK(sig != NULL);
DEBUG_CHECK(pubkey != NULL);

secp256k1_scalar_set_b32(&m, msg32, NULL);

if (!secp256k1_eckey_pubkey_parse(&q, pubkey, pubkeylen)) {
return -1;
}
if (!secp256k1_ecdsa_sig_parse(&s, sig, siglen)) {
return -2;
}
if (!secp256k1_ecdsa_sig_verify(&s, &q, &m)) {
return 0;
if (secp256k1_eckey_pubkey_parse(&q, pubkey, pubkeylen)) {
if (secp256k1_ecdsa_sig_parse(&s, sig, siglen)) {
if (secp256k1_ecdsa_sig_verify(&s, &q, &m)) {
/* success is 1, all other values are fail */
ret = 1;
} else {
ret = 0;
}
} else {
ret = -2;
}
} else {
ret = -1;
}
/* success is 1, all other values are fail */
return 1;

return ret;
}

static int nonce_function_rfc6979(unsigned char *nonce32, const unsigned char *msg32, const unsigned char *key32, unsigned int counter, const void *data) {
Expand Down Expand Up @@ -88,35 +94,34 @@ int secp256k1_ecdsa_sign(const unsigned char *msg32, unsigned char *signature, i
}

secp256k1_scalar_set_b32(&sec, seckey, &overflow);
if (overflow || secp256k1_scalar_is_zero(&sec)) {
*signaturelen = 0;
return 0;
}
secp256k1_scalar_set_b32(&msg, msg32, NULL);
while (1) {
unsigned char nonce32[32];
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
if (!ret) {
break;
}
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
memset(nonce32, 0, 32);
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, NULL)) {
/* Fail if the secret key is invalid. */
if (!overflow && !secp256k1_scalar_is_zero(&sec)) {
secp256k1_scalar_set_b32(&msg, msg32, NULL);
while (1) {
unsigned char nonce32[32];
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
if (!ret) {
break;
}
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
memset(nonce32, 0, 32);
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, NULL)) {
break;
}
}
count++;
}
count++;
}
if (ret) {
ret = secp256k1_ecdsa_sig_serialize(signature, signaturelen, &sig);
if (ret) {
ret = secp256k1_ecdsa_sig_serialize(signature, signaturelen, &sig);
}
secp256k1_scalar_clear(&msg);
secp256k1_scalar_clear(&non);
secp256k1_scalar_clear(&sec);
}
if (!ret) {
*signaturelen = 0;
}
secp256k1_scalar_clear(&msg);
secp256k1_scalar_clear(&non);
secp256k1_scalar_clear(&sec);
return ret;
}

Expand All @@ -135,36 +140,35 @@ int secp256k1_ecdsa_sign_compact(const unsigned char *msg32, unsigned char *sig6
}

secp256k1_scalar_set_b32(&sec, seckey, &overflow);
if (overflow || secp256k1_scalar_is_zero(&sec)) {
memset(sig64, 0, 64);
return 0;
}
secp256k1_scalar_set_b32(&msg, msg32, NULL);
while (1) {
unsigned char nonce32[32];
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
if (!ret) {
break;
}
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
memset(nonce32, 0, 32);
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, recid)) {
/* Fail if the secret key is invalid. */
if (!overflow && !secp256k1_scalar_is_zero(&sec)) {
secp256k1_scalar_set_b32(&msg, msg32, NULL);
while (1) {
unsigned char nonce32[32];
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
if (!ret) {
break;
}
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
memset(nonce32, 0, 32);
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, recid)) {
break;
}
}
count++;
}
count++;
}
if (ret) {
secp256k1_scalar_get_b32(sig64, &sig.r);
secp256k1_scalar_get_b32(sig64 + 32, &sig.s);
if (ret) {
secp256k1_scalar_get_b32(sig64, &sig.r);
secp256k1_scalar_get_b32(sig64 + 32, &sig.s);
}
secp256k1_scalar_clear(&msg);
secp256k1_scalar_clear(&non);
secp256k1_scalar_clear(&sec);
}
if (!ret) {
memset(sig64, 0, 64);
}
secp256k1_scalar_clear(&msg);
secp256k1_scalar_clear(&non);
secp256k1_scalar_clear(&sec);
return ret;
}

Expand All @@ -182,17 +186,15 @@ int secp256k1_ecdsa_recover_compact(const unsigned char *msg32, const unsigned c
DEBUG_CHECK(recid >= 0 && recid <= 3);

secp256k1_scalar_set_b32(&sig.r, sig64, &overflow);
if (overflow) {
return 0;
}
secp256k1_scalar_set_b32(&sig.s, sig64 + 32, &overflow);
if (overflow) {
return 0;
}
secp256k1_scalar_set_b32(&m, msg32, NULL);
if (!overflow) {
secp256k1_scalar_set_b32(&sig.s, sig64 + 32, &overflow);
if (!overflow) {
secp256k1_scalar_set_b32(&m, msg32, NULL);

if (secp256k1_ecdsa_sig_recover(&sig, &q, &m, recid)) {
ret = secp256k1_eckey_pubkey_serialize(&q, pubkey, pubkeylen, compressed);
if (secp256k1_ecdsa_sig_recover(&sig, &q, &m, recid)) {
ret = secp256k1_eckey_pubkey_serialize(&q, pubkey, pubkeylen, compressed);
}
}
}
return ret;
}
Expand Down Expand Up @@ -221,36 +223,41 @@ int secp256k1_ec_pubkey_create(unsigned char *pubkey, int *pubkeylen, const unsi
secp256k1_ge_t p;
secp256k1_scalar_t sec;
int overflow;
int ret = 0;
DEBUG_CHECK(secp256k1_ecmult_gen_consts != NULL);
DEBUG_CHECK(pubkey != NULL);
DEBUG_CHECK(pubkeylen != NULL);
DEBUG_CHECK(seckey != NULL);

secp256k1_scalar_set_b32(&sec, seckey, &overflow);
if (overflow) {
if (!overflow) {
secp256k1_ecmult_gen(&pj, &sec);
secp256k1_scalar_clear(&sec);
secp256k1_ge_set_gej(&p, &pj);
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, compressed);
}
if (!ret) {
*pubkeylen = 0;
return 0;
}
secp256k1_ecmult_gen(&pj, &sec);
secp256k1_scalar_clear(&sec);
secp256k1_ge_set_gej(&p, &pj);
return secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, compressed);
return ret;
}

int secp256k1_ec_pubkey_decompress(unsigned char *pubkey, int *pubkeylen) {
secp256k1_ge_t p;
int ret = 0;
DEBUG_CHECK(pubkey != NULL);
DEBUG_CHECK(pubkeylen != NULL);

if (!secp256k1_eckey_pubkey_parse(&p, pubkey, *pubkeylen))
return 0;
return secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, 0);
if (secp256k1_eckey_pubkey_parse(&p, pubkey, *pubkeylen)) {
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, 0);
}
return ret;
}

int secp256k1_ec_privkey_tweak_add(unsigned char *seckey, const unsigned char *tweak) {
secp256k1_scalar_t term;
secp256k1_scalar_t sec;
int ret;
int ret = 0;
int overflow = 0;
DEBUG_CHECK(seckey != NULL);
DEBUG_CHECK(tweak != NULL);
Expand All @@ -271,24 +278,23 @@ int secp256k1_ec_privkey_tweak_add(unsigned char *seckey, const unsigned char *t
int secp256k1_ec_pubkey_tweak_add(unsigned char *pubkey, int pubkeylen, const unsigned char *tweak) {
secp256k1_ge_t p;
secp256k1_scalar_t term;
int ret;
int ret = 0;
int overflow = 0;
DEBUG_CHECK(secp256k1_ecmult_consts != NULL);
DEBUG_CHECK(pubkey != NULL);
DEBUG_CHECK(tweak != NULL);

secp256k1_scalar_set_b32(&term, tweak, &overflow);
if (overflow) {
return 0;
}
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
if (ret) {
ret = secp256k1_eckey_pubkey_tweak_add(&p, &term);
}
if (ret) {
int oldlen = pubkeylen;
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
VERIFY_CHECK(pubkeylen == oldlen);
if (!overflow) {
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
if (ret) {
ret = secp256k1_eckey_pubkey_tweak_add(&p, &term);
}
if (ret) {
int oldlen = pubkeylen;
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
VERIFY_CHECK(pubkeylen == oldlen);
}
}

return ret;
Expand All @@ -297,7 +303,7 @@ int secp256k1_ec_pubkey_tweak_add(unsigned char *pubkey, int pubkeylen, const un
int secp256k1_ec_privkey_tweak_mul(unsigned char *seckey, const unsigned char *tweak) {
secp256k1_scalar_t factor;
secp256k1_scalar_t sec;
int ret;
int ret = 0;
int overflow = 0;
DEBUG_CHECK(seckey != NULL);
DEBUG_CHECK(tweak != NULL);
Expand All @@ -317,32 +323,31 @@ int secp256k1_ec_privkey_tweak_mul(unsigned char *seckey, const unsigned char *t
int secp256k1_ec_pubkey_tweak_mul(unsigned char *pubkey, int pubkeylen, const unsigned char *tweak) {
secp256k1_ge_t p;
secp256k1_scalar_t factor;
int ret;
int ret = 0;
int overflow = 0;
DEBUG_CHECK(secp256k1_ecmult_consts != NULL);
DEBUG_CHECK(pubkey != NULL);
DEBUG_CHECK(tweak != NULL);

secp256k1_scalar_set_b32(&factor, tweak, &overflow);
if (overflow) {
return 0;
}
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
if (ret) {
ret = secp256k1_eckey_pubkey_tweak_mul(&p, &factor);
}
if (ret) {
int oldlen = pubkeylen;
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
VERIFY_CHECK(pubkeylen == oldlen);
if (!overflow) {
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
if (ret) {
ret = secp256k1_eckey_pubkey_tweak_mul(&p, &factor);
}
if (ret) {
int oldlen = pubkeylen;
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
VERIFY_CHECK(pubkeylen == oldlen);
}
}

return ret;
}

int secp256k1_ec_privkey_export(const unsigned char *seckey, unsigned char *privkey, int *privkeylen, int compressed) {
secp256k1_scalar_t key;
int ret;
int ret = 0;
DEBUG_CHECK(seckey != NULL);
DEBUG_CHECK(privkey != NULL);
DEBUG_CHECK(privkeylen != NULL);
Expand All @@ -355,13 +360,14 @@ int secp256k1_ec_privkey_export(const unsigned char *seckey, unsigned char *priv

int secp256k1_ec_privkey_import(unsigned char *seckey, const unsigned char *privkey, int privkeylen) {
secp256k1_scalar_t key;
int ret;
int ret = 0;
DEBUG_CHECK(seckey != NULL);
DEBUG_CHECK(privkey != NULL);

ret = secp256k1_eckey_privkey_parse(&key, privkey, privkeylen);
if (ret)
if (ret) {
secp256k1_scalar_get_b32(seckey, &key);
}
secp256k1_scalar_clear(&key);
return ret;
}

0 comments on commit 0065a8f

Please sign in to comment.