Skip to content

Commit f65f1a4

Browse files
real-or-randomreal-or-random
committed
Be stricter with side effects in VERIFY
Adds a rule to CONTRIBUTING.md and makes the code adhere to it.
1 parent d831168 commit f65f1a4

10 files changed

+92
-76
lines changed

‎CONTRIBUTING.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ In addition, libsecp256k1 tries to maintain the following coding conventions:
4949
* Operations involving secret data should be tested for being constant time with respect to the secrets (see [src/ctime_tests.c](src/ctime_tests.c)).
5050
* Local variables containing secret data should be cleared explicitly to try to delete secrets from memory.
5151
* Use `secp256k1_memcmp_var` instead of `memcmp` (see [#823](https://github.com/bitcoin-core/secp256k1/issues/823)).
52+
* Use `VERIFY_CHECK(cond)` for test-only assertions.
53+
* These are active only when the `VERIFY` macro is defined. The build system defines this macro when building test targets such as the `tests` and `exhaustive_tests` executables, but not when building the actual library.
54+
* If you need additional code lines to prepare the call to `VERIFY_CHECK`, then wrap them and the call into `#ifdef VERIFY ... #endif`. Start a new block (see style conventions below) inside the `#ifdef VERIFY` if appropriate, and suffix `VERIFY`-only variables with `_ver`.
55+
* In any `VERIFY` code, avoid side effects to variables used in non-`VERIFY` code. Note that `VERIFY_CHECK(cond)` is a no-op if `VERIFY` is not defined, so avoid also side effects in `cond`.
5256

5357
#### Style conventions
5458

‎src/ecmult_const_impl.h

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -212,10 +212,13 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
212212
secp256k1_scalar_add(&v2, &v2, &S_OFFSET);
213213

214214
#ifdef VERIFY
215-
/* Verify that v1 and v2 are in range [0, 2^129-1]. */
216-
for (i = 129; i < 256; ++i) {
217-
VERIFY_CHECK(secp256k1_scalar_get_bits(&v1, i, 1) == 0);
218-
VERIFY_CHECK(secp256k1_scalar_get_bits(&v2, i, 1) == 0);
215+
{
216+
int i_ver;
217+
/* Verify that v1 and v2 are in range [0, 2^129-1]. */
218+
for (i_ver = 129; i_ver < 256; ++i_ver) {
219+
VERIFY_CHECK(secp256k1_scalar_get_bits(&v1, i_ver, 1) == 0);
220+
VERIFY_CHECK(secp256k1_scalar_get_bits(&v2, i_ver, 1) == 0);
221+
}
219222
}
220223
#endif
221224

‎src/ecmult_impl.h

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -202,15 +202,14 @@ static int secp256k1_ecmult_wnaf(int *wnaf, int len, const secp256k1_scalar *a,
202202

203203
bit += now;
204204
}
205+
VERIFY_CHECK(carry == 0);
206+
205207
#ifdef VERIFY
206208
{
207-
int verify_bit = bit;
208-
209-
VERIFY_CHECK(carry == 0);
210-
211-
while (verify_bit < 256) {
212-
VERIFY_CHECK(secp256k1_scalar_get_bits(&s, verify_bit, 1) == 0);
213-
verify_bit++;
209+
int bit_ver = bit;
210+
while (bit_ver < 256) {
211+
VERIFY_CHECK(secp256k1_scalar_get_bits(&s, bit_ver, 1) == 0);
212+
bit_ver++;
214213
}
215214
}
216215
#endif

‎src/field_impl.h

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,13 @@ static int secp256k1_fe_sqrt(secp256k1_fe * SECP256K1_RESTRICT r, const secp256k
132132
ret = secp256k1_fe_equal(&t1, a);
133133

134134
#ifdef VERIFY
135-
if (!ret) {
136-
secp256k1_fe_negate(&t1, &t1, 1);
137-
secp256k1_fe_normalize_var(&t1);
138-
VERIFY_CHECK(secp256k1_fe_equal(&t1, a));
135+
{
136+
secp256k1_fe t1_ver;
137+
if (!ret) {
138+
secp256k1_fe_negate(&t1_ver, &t1, 1);
139+
secp256k1_fe_normalize_var(&t1_ver);
140+
VERIFY_CHECK(secp256k1_fe_equal(&t1_ver, a));
141+
}
139142
}
140143
#endif
141144
return ret;

‎src/group_impl.h

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -198,8 +198,11 @@ static void secp256k1_ge_set_all_gej_var(secp256k1_ge *r, const secp256k1_gej *a
198198
size_t i;
199199
size_t last_i = SIZE_MAX;
200200
#ifdef VERIFY
201-
for (i = 0; i < len; i++) {
202-
SECP256K1_GEJ_VERIFY(&a[i]);
201+
{
202+
size_t i_ver;
203+
for (i_ver = 0; i_ver < len; i_ver++) {
204+
SECP256K1_GEJ_VERIFY(&a[i_ver]);
205+
}
203206
}
204207
#endif
205208

@@ -240,8 +243,11 @@ static void secp256k1_ge_set_all_gej_var(secp256k1_ge *r, const secp256k1_gej *a
240243
}
241244

242245
#ifdef VERIFY
243-
for (i = 0; i < len; i++) {
244-
SECP256K1_GE_VERIFY(&r[i]);
246+
{
247+
size_t i_ver;
248+
for (i_ver = 0; i_ver < len; i_ver++) {
249+
SECP256K1_GE_VERIFY(&r[i_ver]);
250+
}
245251
}
246252
#endif
247253
}
@@ -250,9 +256,12 @@ static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const se
250256
size_t i;
251257
secp256k1_fe zs;
252258
#ifdef VERIFY
253-
for (i = 0; i < len; i++) {
254-
SECP256K1_GE_VERIFY(&a[i]);
255-
SECP256K1_FE_VERIFY(&zr[i]);
259+
{
260+
size_t i_ver;
261+
for (i_ver = 0; i_ver < len; i_ver++) {
262+
SECP256K1_GE_VERIFY(&a[i_ver]);
263+
SECP256K1_FE_VERIFY(&zr[i_ver]);
264+
}
256265
}
257266
#endif
258267

@@ -273,8 +282,11 @@ static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const se
273282
}
274283

275284
#ifdef VERIFY
276-
for (i = 0; i < len; i++) {
277-
SECP256K1_GE_VERIFY(&a[i]);
285+
{
286+
size_t i_ver;
287+
for (i_ver = 0; i_ver < len; i_ver++) {
288+
SECP256K1_GE_VERIFY(&a[i_ver]);
289+
}
278290
}
279291
#endif
280292
}

‎src/modinv32_impl.h

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,16 @@ static void secp256k1_modinv32_normalize_30(secp256k1_modinv32_signed30 *r, int3
6767
volatile int32_t cond_add, cond_negate;
6868

6969
#ifdef VERIFY
70-
/* Verify that all limbs are in range (-2^30,2^30). */
71-
int i;
72-
for (i = 0; i < 9; ++i) {
73-
VERIFY_CHECK(r->v[i] >= -M30);
74-
VERIFY_CHECK(r->v[i] <= M30);
70+
{
71+
/* Verify that all limbs are in range (-2^30,2^30). */
72+
int i_ver;
73+
for (i_ver = 0; i_ver < 9; ++i_ver) {
74+
VERIFY_CHECK(r->v[i_ver] >= -M30);
75+
VERIFY_CHECK(r->v[i_ver] <= M30);
76+
}
77+
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(r, 9, &modinfo->modulus, -2) > 0); /* r > -2*modulus */
78+
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(r, 9, &modinfo->modulus, 1) < 0); /* r < modulus */
7579
}
76-
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(r, 9, &modinfo->modulus, -2) > 0); /* r > -2*modulus */
77-
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(r, 9, &modinfo->modulus, 1) < 0); /* r < modulus */
7880
#endif
7981

8082
/* In a first step, add the modulus if the input is negative, and then negate if requested.
@@ -586,7 +588,7 @@ static void secp256k1_modinv32_var(secp256k1_modinv32_signed30 *x, const secp256
586588
secp256k1_modinv32_signed30 f = modinfo->modulus;
587589
secp256k1_modinv32_signed30 g = *x;
588590
#ifdef VERIFY
589-
int i = 0;
591+
int i_ver = 0;
590592
#endif
591593
int j, len = 9;
592594
int32_t eta = -1; /* eta = -delta; delta is initially 1 (faster for the variable-time code) */
@@ -631,7 +633,7 @@ static void secp256k1_modinv32_var(secp256k1_modinv32_signed30 *x, const secp256
631633
--len;
632634
}
633635

634-
VERIFY_CHECK(++i < 25); /* We should never need more than 25*30 = 750 divsteps */
636+
VERIFY_CHECK(++i_ver < 25); /* We should never need more than 25*30 = 750 divsteps */
635637
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
636638
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(&f, len, &modinfo->modulus, 1) <= 0); /* f <= modulus */
637639
VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(&g, len, &modinfo->modulus, -1) > 0); /* g > -modulus */

‎src/modinv64_impl.h

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,16 @@ static void secp256k1_modinv64_normalize_62(secp256k1_modinv64_signed62 *r, int6
9191
volatile int64_t cond_add, cond_negate;
9292

9393
#ifdef VERIFY
94-
/* Verify that all limbs are in range (-2^62,2^62). */
95-
int i;
96-
for (i = 0; i < 5; ++i) {
97-
VERIFY_CHECK(r->v[i] >= -M62);
98-
VERIFY_CHECK(r->v[i] <= M62);
94+
{
95+
/* Verify that all limbs are in range (-2^62,2^62). */
96+
int i_ver;
97+
for (i_ver = 0; i_ver < 5; ++i_ver) {
98+
VERIFY_CHECK(r->v[i_ver] >= -M62);
99+
VERIFY_CHECK(r->v[i_ver] <= M62);
100+
}
101+
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(r, 5, &modinfo->modulus, -2) > 0); /* r > -2*modulus */
102+
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(r, 5, &modinfo->modulus, 1) < 0); /* r < modulus */
99103
}
100-
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(r, 5, &modinfo->modulus, -2) > 0); /* r > -2*modulus */
101-
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(r, 5, &modinfo->modulus, 1) < 0); /* r < modulus */
102104
#endif
103105

104106
/* In a first step, add the modulus if the input is negative, and then negate if requested.
@@ -642,7 +644,7 @@ static void secp256k1_modinv64_var(secp256k1_modinv64_signed62 *x, const secp256
642644
secp256k1_modinv64_signed62 f = modinfo->modulus;
643645
secp256k1_modinv64_signed62 g = *x;
644646
#ifdef VERIFY
645-
int i = 0;
647+
int i_ver = 0;
646648
#endif
647649
int j, len = 5;
648650
int64_t eta = -1; /* eta = -delta; delta is initially 1 */
@@ -686,7 +688,7 @@ static void secp256k1_modinv64_var(secp256k1_modinv64_signed62 *x, const secp256
686688
--len;
687689
}
688690

689-
VERIFY_CHECK(++i < 12); /* We should never need more than 12*62 = 744 divsteps */
691+
VERIFY_CHECK(++i_ver < 12); /* We should never need more than 12*62 = 744 divsteps */
690692
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
691693
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, 1) <= 0); /* f <= modulus */
692694
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&g, len, &modinfo->modulus, -1) > 0); /* g > -modulus */

‎src/modules/ellswift/main_impl.h

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ static int secp256k1_ellswift_xswiftec_inv_var(secp256k1_fe *t, const secp256k1_
187187
* - If (c & 5) = 5: return -w*(c4*u + v).
188188
*/
189189
secp256k1_fe x = *x_in, u = *u_in, g, v, s, m, r, q;
190-
int ret;
190+
int ret_ver;
191191

192192
secp256k1_fe_normalize_weak(&x);
193193
secp256k1_fe_normalize_weak(&u);
@@ -266,11 +266,11 @@ static int secp256k1_ellswift_xswiftec_inv_var(secp256k1_fe *t, const secp256k1_
266266
secp256k1_fe_mul(&q, &q, &s); /* q = s*(4*(u^3+7)+3*u^2*s) */
267267
secp256k1_fe_negate(&q, &q, 1); /* q = -s*(4*(u^3+7)+3*u^2*s) */
268268
if (!secp256k1_fe_is_square_var(&q)) return 0;
269-
ret = secp256k1_fe_sqrt(&r, &q); /* r = sqrt(-s*(4*(u^3+7)+3*u^2*s)) */
269+
ret_ver = secp256k1_fe_sqrt(&r, &q); /* r = sqrt(-s*(4*(u^3+7)+3*u^2*s)) */
270270
#ifdef VERIFY
271-
VERIFY_CHECK(ret);
271+
VERIFY_CHECK(ret_ver);
272272
#else
273-
(void)ret;
273+
(void)ret_ver;
274274
#endif
275275

276276
/* If (c & 1) = 1 and r = 0, fail. */
@@ -287,8 +287,8 @@ static int secp256k1_ellswift_xswiftec_inv_var(secp256k1_fe *t, const secp256k1_
287287
}
288288

289289
/* Let w = sqrt(s). */
290-
ret = secp256k1_fe_sqrt(&m, &s); /* m = sqrt(s) = w */
291-
VERIFY_CHECK(ret);
290+
ret_ver = secp256k1_fe_sqrt(&m, &s); /* m = sqrt(s) = w */
291+
VERIFY_CHECK(ret_ver);
292292

293293
/* Return logic. */
294294
if ((c & 5) == 0 || (c & 5) == 5) {
@@ -311,7 +311,7 @@ static void secp256k1_ellswift_prng(unsigned char* out32, const secp256k1_sha256
311311
secp256k1_sha256 hash = *hasher;
312312
unsigned char buf4[4];
313313
#ifdef VERIFY
314-
size_t blocks = hash.bytes >> 6;
314+
size_t blocks_ver = hash.bytes >> 6;
315315
#endif
316316
buf4[0] = cnt;
317317
buf4[1] = cnt >> 8;
@@ -321,7 +321,7 @@ static void secp256k1_ellswift_prng(unsigned char* out32, const secp256k1_sha256
321321
secp256k1_sha256_finalize(&hash, out32);
322322

323323
/* Writing and finalizing together should trigger exactly one SHA256 compression. */
324-
VERIFY_CHECK(((hash.bytes) >> 6) == (blocks + 1));
324+
VERIFY_CHECK(((hash.bytes) >> 6) == (blocks_ver + 1));
325325
}
326326

327327
/** Find an ElligatorSwift encoding (u, t) for X coordinate x, and random Y coordinate.
@@ -407,17 +407,17 @@ int secp256k1_ellswift_encode(const secp256k1_context *ctx, unsigned char *ell64
407407
secp256k1_fe t;
408408
unsigned char p64[64] = {0};
409409
size_t ser_size;
410-
int ser_ret;
410+
int ser_ret_ver;
411411
secp256k1_sha256 hash;
412412

413413
/* Set up hasher state; the used RNG is H(pubkey || "\x00"*31 || rnd32 || cnt++), using
414414
* BIP340 tagged hash with tag "secp256k1_ellswift_encode". */
415415
secp256k1_ellswift_sha256_init_encode(&hash);
416-
ser_ret = secp256k1_eckey_pubkey_serialize(&p, p64, &ser_size, 1);
416+
ser_ret_ver = secp256k1_eckey_pubkey_serialize(&p, p64, &ser_size, 1);
417417
#ifdef VERIFY
418-
VERIFY_CHECK(ser_ret && ser_size == 33);
418+
VERIFY_CHECK(ser_ret_ver && ser_size == 33);
419419
#else
420-
(void)ser_ret;
420+
(void)ser_ret_ver;
421421
#endif
422422
secp256k1_sha256_write(&hash, p64, sizeof(p64));
423423
secp256k1_sha256_write(&hash, rnd32, 32);

‎src/scalar_4x64_impl.h

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -227,12 +227,15 @@ static void secp256k1_scalar_half(secp256k1_scalar *r, const secp256k1_scalar *a
227227
r->d[2] = secp256k1_u128_to_u64(&t); secp256k1_u128_rshift(&t, 64);
228228
r->d[3] = secp256k1_u128_to_u64(&t) + (a->d[3] >> 1) + (SECP256K1_N_H_3 & mask);
229229
#ifdef VERIFY
230-
/* The line above only computed the bottom 64 bits of r->d[3]; redo the computation
231-
* in full 128 bits to make sure the top 64 bits are indeed zero. */
232-
secp256k1_u128_accum_u64(&t, a->d[3] >> 1);
233-
secp256k1_u128_accum_u64(&t, SECP256K1_N_H_3 & mask);
234-
secp256k1_u128_rshift(&t, 64);
235-
VERIFY_CHECK(secp256k1_u128_to_u64(&t) == 0);
230+
{
231+
/* The line above only computed the bottom 64 bits of r->d[3]; redo the computation
232+
* in full 128 bits to make sure the top 64 bits are indeed zero. */
233+
secp256k1_uint128 t_ver = t;
234+
secp256k1_u128_accum_u64(&t_ver, a->d[3] >> 1);
235+
secp256k1_u128_accum_u64(&t_ver, SECP256K1_N_H_3 & mask);
236+
secp256k1_u128_rshift(&t_ver, 64);
237+
VERIFY_CHECK(secp256k1_u128_to_u64(&t_ver) == 0);
238+
}
236239

237240
SECP256K1_SCALAR_VERIFY(r);
238241
#endif
@@ -969,32 +972,26 @@ static const secp256k1_modinv64_modinfo secp256k1_const_modinfo_scalar = {
969972

970973
static void secp256k1_scalar_inverse(secp256k1_scalar *r, const secp256k1_scalar *x) {
971974
secp256k1_modinv64_signed62 s;
972-
#ifdef VERIFY
973-
int zero_in = secp256k1_scalar_is_zero(x);
974-
#endif
975975
SECP256K1_SCALAR_VERIFY(x);
976976

977977
secp256k1_scalar_to_signed62(&s, x);
978978
secp256k1_modinv64(&s, &secp256k1_const_modinfo_scalar);
979979
secp256k1_scalar_from_signed62(r, &s);
980980

981+
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == secp256k1_scalar_is_zero(x));
981982
SECP256K1_SCALAR_VERIFY(r);
982-
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == zero_in);
983983
}
984984

985985
static void secp256k1_scalar_inverse_var(secp256k1_scalar *r, const secp256k1_scalar *x) {
986986
secp256k1_modinv64_signed62 s;
987-
#ifdef VERIFY
988-
int zero_in = secp256k1_scalar_is_zero(x);
989-
#endif
990987
SECP256K1_SCALAR_VERIFY(x);
991988

992989
secp256k1_scalar_to_signed62(&s, x);
993990
secp256k1_modinv64_var(&s, &secp256k1_const_modinfo_scalar);
994991
secp256k1_scalar_from_signed62(r, &s);
995992

993+
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == secp256k1_scalar_is_zero(x));
996994
SECP256K1_SCALAR_VERIFY(r);
997-
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == zero_in);
998995
}
999996

1000997
SECP256K1_INLINE static int secp256k1_scalar_is_even(const secp256k1_scalar *a) {

‎src/scalar_8x32_impl.h

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -789,32 +789,26 @@ static const secp256k1_modinv32_modinfo secp256k1_const_modinfo_scalar = {
789789

790790
static void secp256k1_scalar_inverse(secp256k1_scalar *r, const secp256k1_scalar *x) {
791791
secp256k1_modinv32_signed30 s;
792-
#ifdef VERIFY
793-
int zero_in = secp256k1_scalar_is_zero(x);
794-
#endif
795792
SECP256K1_SCALAR_VERIFY(x);
796793

797794
secp256k1_scalar_to_signed30(&s, x);
798795
secp256k1_modinv32(&s, &secp256k1_const_modinfo_scalar);
799796
secp256k1_scalar_from_signed30(r, &s);
800797

798+
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == secp256k1_scalar_is_zero(x));
801799
SECP256K1_SCALAR_VERIFY(r);
802-
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == zero_in);
803800
}
804801

805802
static void secp256k1_scalar_inverse_var(secp256k1_scalar *r, const secp256k1_scalar *x) {
806803
secp256k1_modinv32_signed30 s;
807-
#ifdef VERIFY
808-
int zero_in = secp256k1_scalar_is_zero(x);
809-
#endif
810804
SECP256K1_SCALAR_VERIFY(x);
811805

812806
secp256k1_scalar_to_signed30(&s, x);
813807
secp256k1_modinv32_var(&s, &secp256k1_const_modinfo_scalar);
814808
secp256k1_scalar_from_signed30(r, &s);
815809

810+
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == secp256k1_scalar_is_zero(x));
816811
SECP256K1_SCALAR_VERIFY(r);
817-
VERIFY_CHECK(secp256k1_scalar_is_zero(r) == zero_in);
818812
}
819813

820814
SECP256K1_INLINE static int secp256k1_scalar_is_even(const secp256k1_scalar *a) {

0 commit comments

Comments
 (0)