Skip to content

Commit 0065a8f

Browse files
gmaxwellgmaxwell
committed
Eliminate multiple-returns from secp256k1.c.
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.
1 parent 354ffa3 commit 0065a8f

File tree

1 file changed

+109
-103
lines changed

1 file changed

+109
-103
lines changed

‎src/secp256k1.c‎

Lines changed: 109 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -37,24 +37,30 @@ int secp256k1_ecdsa_verify(const unsigned char *msg32, const unsigned char *sig,
3737
secp256k1_ge_t q;
3838
secp256k1_ecdsa_sig_t s;
3939
secp256k1_scalar_t m;
40+
int ret = -3;
4041
DEBUG_CHECK(secp256k1_ecmult_consts != NULL);
4142
DEBUG_CHECK(msg32 != NULL);
4243
DEBUG_CHECK(sig != NULL);
4344
DEBUG_CHECK(pubkey != NULL);
4445

4546
secp256k1_scalar_set_b32(&m, msg32, NULL);
4647

47-
if (!secp256k1_eckey_pubkey_parse(&q, pubkey, pubkeylen)) {
48-
return -1;
49-
}
50-
if (!secp256k1_ecdsa_sig_parse(&s, sig, siglen)) {
51-
return -2;
52-
}
53-
if (!secp256k1_ecdsa_sig_verify(&s, &q, &m)) {
54-
return 0;
48+
if (secp256k1_eckey_pubkey_parse(&q, pubkey, pubkeylen)) {
49+
if (secp256k1_ecdsa_sig_parse(&s, sig, siglen)) {
50+
if (secp256k1_ecdsa_sig_verify(&s, &q, &m)) {
51+
/* success is 1, all other values are fail */
52+
ret = 1;
53+
} else {
54+
ret = 0;
55+
}
56+
} else {
57+
ret = -2;
58+
}
59+
} else {
60+
ret = -1;
5561
}
56-
/* success is 1, all other values are fail */
57-
return 1;
62+
63+
return ret;
5864
}
5965

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

9096
secp256k1_scalar_set_b32(&sec, seckey, &overflow);
91-
if (overflow || secp256k1_scalar_is_zero(&sec)) {
92-
*signaturelen = 0;
93-
return 0;
94-
}
95-
secp256k1_scalar_set_b32(&msg, msg32, NULL);
96-
while (1) {
97-
unsigned char nonce32[32];
98-
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
99-
if (!ret) {
100-
break;
101-
}
102-
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
103-
memset(nonce32, 0, 32);
104-
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
105-
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, NULL)) {
97+
/* Fail if the secret key is invalid. */
98+
if (!overflow && !secp256k1_scalar_is_zero(&sec)) {
99+
secp256k1_scalar_set_b32(&msg, msg32, NULL);
100+
while (1) {
101+
unsigned char nonce32[32];
102+
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
103+
if (!ret) {
106104
break;
107105
}
106+
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
107+
memset(nonce32, 0, 32);
108+
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
109+
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, NULL)) {
110+
break;
111+
}
112+
}
113+
count++;
108114
}
109-
count++;
110-
}
111-
if (ret) {
112-
ret = secp256k1_ecdsa_sig_serialize(signature, signaturelen, &sig);
115+
if (ret) {
116+
ret = secp256k1_ecdsa_sig_serialize(signature, signaturelen, &sig);
117+
}
118+
secp256k1_scalar_clear(&msg);
119+
secp256k1_scalar_clear(&non);
120+
secp256k1_scalar_clear(&sec);
113121
}
114122
if (!ret) {
115123
*signaturelen = 0;
116124
}
117-
secp256k1_scalar_clear(&msg);
118-
secp256k1_scalar_clear(&non);
119-
secp256k1_scalar_clear(&sec);
120125
return ret;
121126
}
122127

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

137142
secp256k1_scalar_set_b32(&sec, seckey, &overflow);
138-
if (overflow || secp256k1_scalar_is_zero(&sec)) {
139-
memset(sig64, 0, 64);
140-
return 0;
141-
}
142-
secp256k1_scalar_set_b32(&msg, msg32, NULL);
143-
while (1) {
144-
unsigned char nonce32[32];
145-
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
146-
if (!ret) {
147-
break;
148-
}
149-
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
150-
memset(nonce32, 0, 32);
151-
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
152-
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, recid)) {
143+
/* Fail if the secret key is invalid. */
144+
if (!overflow && !secp256k1_scalar_is_zero(&sec)) {
145+
secp256k1_scalar_set_b32(&msg, msg32, NULL);
146+
while (1) {
147+
unsigned char nonce32[32];
148+
ret = noncefp(nonce32, msg32, seckey, count, noncedata);
149+
if (!ret) {
153150
break;
154151
}
152+
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
153+
memset(nonce32, 0, 32);
154+
if (!secp256k1_scalar_is_zero(&non) && !overflow) {
155+
if (secp256k1_ecdsa_sig_sign(&sig, &sec, &msg, &non, recid)) {
156+
break;
157+
}
158+
}
159+
count++;
155160
}
156-
count++;
157-
}
158-
if (ret) {
159-
secp256k1_scalar_get_b32(sig64, &sig.r);
160-
secp256k1_scalar_get_b32(sig64 + 32, &sig.s);
161+
if (ret) {
162+
secp256k1_scalar_get_b32(sig64, &sig.r);
163+
secp256k1_scalar_get_b32(sig64 + 32, &sig.s);
164+
}
165+
secp256k1_scalar_clear(&msg);
166+
secp256k1_scalar_clear(&non);
167+
secp256k1_scalar_clear(&sec);
161168
}
162169
if (!ret) {
163170
memset(sig64, 0, 64);
164171
}
165-
secp256k1_scalar_clear(&msg);
166-
secp256k1_scalar_clear(&non);
167-
secp256k1_scalar_clear(&sec);
168172
return ret;
169173
}
170174

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

184188
secp256k1_scalar_set_b32(&sig.r, sig64, &overflow);
185-
if (overflow) {
186-
return 0;
187-
}
188-
secp256k1_scalar_set_b32(&sig.s, sig64 + 32, &overflow);
189-
if (overflow) {
190-
return 0;
191-
}
192-
secp256k1_scalar_set_b32(&m, msg32, NULL);
189+
if (!overflow) {
190+
secp256k1_scalar_set_b32(&sig.s, sig64 + 32, &overflow);
191+
if (!overflow) {
192+
secp256k1_scalar_set_b32(&m, msg32, NULL);
193193

194-
if (secp256k1_ecdsa_sig_recover(&sig, &q, &m, recid)) {
195-
ret = secp256k1_eckey_pubkey_serialize(&q, pubkey, pubkeylen, compressed);
194+
if (secp256k1_ecdsa_sig_recover(&sig, &q, &m, recid)) {
195+
ret = secp256k1_eckey_pubkey_serialize(&q, pubkey, pubkeylen, compressed);
196+
}
197+
}
196198
}
197199
return ret;
198200
}
@@ -221,36 +223,41 @@ int secp256k1_ec_pubkey_create(unsigned char *pubkey, int *pubkeylen, const unsi
221223
secp256k1_ge_t p;
222224
secp256k1_scalar_t sec;
223225
int overflow;
226+
int ret = 0;
224227
DEBUG_CHECK(secp256k1_ecmult_gen_consts != NULL);
225228
DEBUG_CHECK(pubkey != NULL);
226229
DEBUG_CHECK(pubkeylen != NULL);
227230
DEBUG_CHECK(seckey != NULL);
228231

229232
secp256k1_scalar_set_b32(&sec, seckey, &overflow);
230-
if (overflow) {
233+
if (!overflow) {
234+
secp256k1_ecmult_gen(&pj, &sec);
235+
secp256k1_scalar_clear(&sec);
236+
secp256k1_ge_set_gej(&p, &pj);
237+
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, compressed);
238+
}
239+
if (!ret) {
231240
*pubkeylen = 0;
232-
return 0;
233241
}
234-
secp256k1_ecmult_gen(&pj, &sec);
235-
secp256k1_scalar_clear(&sec);
236-
secp256k1_ge_set_gej(&p, &pj);
237-
return secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, compressed);
242+
return ret;
238243
}
239244

240245
int secp256k1_ec_pubkey_decompress(unsigned char *pubkey, int *pubkeylen) {
241246
secp256k1_ge_t p;
247+
int ret = 0;
242248
DEBUG_CHECK(pubkey != NULL);
243249
DEBUG_CHECK(pubkeylen != NULL);
244250

245-
if (!secp256k1_eckey_pubkey_parse(&p, pubkey, *pubkeylen))
246-
return 0;
247-
return secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, 0);
251+
if (secp256k1_eckey_pubkey_parse(&p, pubkey, *pubkeylen)) {
252+
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, pubkeylen, 0);
253+
}
254+
return ret;
248255
}
249256

250257
int secp256k1_ec_privkey_tweak_add(unsigned char *seckey, const unsigned char *tweak) {
251258
secp256k1_scalar_t term;
252259
secp256k1_scalar_t sec;
253-
int ret;
260+
int ret = 0;
254261
int overflow = 0;
255262
DEBUG_CHECK(seckey != NULL);
256263
DEBUG_CHECK(tweak != NULL);
@@ -271,24 +278,23 @@ int secp256k1_ec_privkey_tweak_add(unsigned char *seckey, const unsigned char *t
271278
int secp256k1_ec_pubkey_tweak_add(unsigned char *pubkey, int pubkeylen, const unsigned char *tweak) {
272279
secp256k1_ge_t p;
273280
secp256k1_scalar_t term;
274-
int ret;
281+
int ret = 0;
275282
int overflow = 0;
276283
DEBUG_CHECK(secp256k1_ecmult_consts != NULL);
277284
DEBUG_CHECK(pubkey != NULL);
278285
DEBUG_CHECK(tweak != NULL);
279286

280287
secp256k1_scalar_set_b32(&term, tweak, &overflow);
281-
if (overflow) {
282-
return 0;
283-
}
284-
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
285-
if (ret) {
286-
ret = secp256k1_eckey_pubkey_tweak_add(&p, &term);
287-
}
288-
if (ret) {
289-
int oldlen = pubkeylen;
290-
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
291-
VERIFY_CHECK(pubkeylen == oldlen);
288+
if (!overflow) {
289+
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
290+
if (ret) {
291+
ret = secp256k1_eckey_pubkey_tweak_add(&p, &term);
292+
}
293+
if (ret) {
294+
int oldlen = pubkeylen;
295+
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
296+
VERIFY_CHECK(pubkeylen == oldlen);
297+
}
292298
}
293299

294300
return ret;
@@ -297,7 +303,7 @@ int secp256k1_ec_pubkey_tweak_add(unsigned char *pubkey, int pubkeylen, const un
297303
int secp256k1_ec_privkey_tweak_mul(unsigned char *seckey, const unsigned char *tweak) {
298304
secp256k1_scalar_t factor;
299305
secp256k1_scalar_t sec;
300-
int ret;
306+
int ret = 0;
301307
int overflow = 0;
302308
DEBUG_CHECK(seckey != NULL);
303309
DEBUG_CHECK(tweak != NULL);
@@ -317,32 +323,31 @@ int secp256k1_ec_privkey_tweak_mul(unsigned char *seckey, const unsigned char *t
317323
int secp256k1_ec_pubkey_tweak_mul(unsigned char *pubkey, int pubkeylen, const unsigned char *tweak) {
318324
secp256k1_ge_t p;
319325
secp256k1_scalar_t factor;
320-
int ret;
326+
int ret = 0;
321327
int overflow = 0;
322328
DEBUG_CHECK(secp256k1_ecmult_consts != NULL);
323329
DEBUG_CHECK(pubkey != NULL);
324330
DEBUG_CHECK(tweak != NULL);
325331

326332
secp256k1_scalar_set_b32(&factor, tweak, &overflow);
327-
if (overflow) {
328-
return 0;
329-
}
330-
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
331-
if (ret) {
332-
ret = secp256k1_eckey_pubkey_tweak_mul(&p, &factor);
333-
}
334-
if (ret) {
335-
int oldlen = pubkeylen;
336-
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
337-
VERIFY_CHECK(pubkeylen == oldlen);
333+
if (!overflow) {
334+
ret = secp256k1_eckey_pubkey_parse(&p, pubkey, pubkeylen);
335+
if (ret) {
336+
ret = secp256k1_eckey_pubkey_tweak_mul(&p, &factor);
337+
}
338+
if (ret) {
339+
int oldlen = pubkeylen;
340+
ret = secp256k1_eckey_pubkey_serialize(&p, pubkey, &pubkeylen, oldlen <= 33);
341+
VERIFY_CHECK(pubkeylen == oldlen);
342+
}
338343
}
339344

340345
return ret;
341346
}
342347

343348
int secp256k1_ec_privkey_export(const unsigned char *seckey, unsigned char *privkey, int *privkeylen, int compressed) {
344349
secp256k1_scalar_t key;
345-
int ret;
350+
int ret = 0;
346351
DEBUG_CHECK(seckey != NULL);
347352
DEBUG_CHECK(privkey != NULL);
348353
DEBUG_CHECK(privkeylen != NULL);
@@ -355,13 +360,14 @@ int secp256k1_ec_privkey_export(const unsigned char *seckey, unsigned char *priv
355360

356361
int secp256k1_ec_privkey_import(unsigned char *seckey, const unsigned char *privkey, int privkeylen) {
357362
secp256k1_scalar_t key;
358-
int ret;
363+
int ret = 0;
359364
DEBUG_CHECK(seckey != NULL);
360365
DEBUG_CHECK(privkey != NULL);
361366

362367
ret = secp256k1_eckey_privkey_parse(&key, privkey, privkeylen);
363-
if (ret)
368+
if (ret) {
364369
secp256k1_scalar_get_b32(seckey, &key);
370+
}
365371
secp256k1_scalar_clear(&key);
366372
return ret;
367373
}

0 commit comments

Comments
 (0)