gcc emits -Warray-bounds warnings

hebasto commented at 2:13 pm on October 18, 2020: member

While compiling the bitcoin master branch (https://github.com/bitcoin/bitcoin/commit/80c8a02f1b4f6ad2b5c02595d66a74db22373ed4) having the following warnings:

 0$ gcc --version
 1gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
 2Copyright (C) 2019 Free Software Foundation, Inc.
 3This is free software; see the source for copying conditions.  There is NO
 4warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 5
 6$ make > /dev/null
 7In file included from src/secp256k1.c:16:
 8src/ecmult_impl.h: In function ‘secp256k1_ecmult’:
 9src/ecmult_impl.h:496:48: warning: array subscript [1, 268435456] is outside array bounds of ‘struct secp256k1_strauss_point_state[1]’ [-Warray-bounds]
10  496 |             secp256k1_gej tmp = a[state->ps[np].input_pos];
11      |                                   ~~~~~~~~~~~~~^~~~~~~~~~
12src/ecmult_impl.h:565:42: note: while referencing ‘ps’
13  565 |     struct secp256k1_strauss_point_state ps[1];
14      |                                          ^~
15src/ecmult_impl.h:502:139: warning: array subscript [1, 268435456] is outside array bounds of ‘struct secp256k1_strauss_point_state[1]’ [-Warray-bounds]
16  502 |             secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
17      |                                                                                                                              ~~~~~~~~~~~~~^~~~~~~~~~
18src/ecmult_impl.h:565:42: note: while referencing ‘ps’
19  565 |     struct secp256k1_strauss_point_state ps[1];
20      |                                          ^~

elichai commented at 2:44 pm on October 18, 2020: contributor

This looks like a false positive. on line 574 we pass num=1 then according to the loop on 460 no will also be 1 when the loop ends. then on line 495 the loop will never enter because the np = 1; np < no condition is false (1<1)

real-or-random commented at 1:26 pm on October 20, 2020: contributor

Yes, I think that’s a false positive… I’ve seen this one too. It’s gone on GCC 10 (tested with 10.2), even with -Warray-bounds=2. Should we do something about his?

What’s not great however is the other call to secp256k1_ecmult_strauss_wnaf: (This is currently unused dead code for future applications, so this is not at all an issue currently): https://github.com/bitcoin-core/secp256k1/blob/c6b6b8f1bb044d7d1aa065ebb674adde98a36a8e/src/ecmult_impl.h#L615

Here we pass an unsigned size_t n_points as the 4th argument (int num) to secp256k1_ecmult_strauss_wnaf. I suggest we change all the counter variables inside secp256k1_ecmult_strauss_wnaf to size_t. Apparently we do this in the corresponding code for pippenger but not here. See around https://github.com/bitcoin-core/secp256k1/blob/c6b6b8f1bb044d7d1aa065ebb674adde98a36a8e/src/ecmult_impl.h#L715

gmaxwell commented at 7:08 am on October 21, 2020: contributor

IIRC it’s trivial to get rid of by refactoring the loop into a do while… sorry, I noticed this months ago (its only in compilers I don’t usually use and only happens with endo enabled) and talked to sipa about it but neither of us opened a pull request. (I had a patch in a pastebin, but it expired…)

real-or-random commented at 1:52 pm on October 26, 2020: contributor

There’s a small chance that this is related to #839. @hebasto Can you check if the warning disappears with this PR?

real-or-random referenced this in commit 4915b48e35 on Oct 26, 2020

real-or-random referenced this in commit f1c9131c79 on Oct 26, 2020

real-or-random cross-referenced this on Oct 26, 2020 from issue Avoids a potentially shortening size_t to int cast in strauss_wnaf_ by real-or-random

real-or-random commented at 2:01 pm on October 26, 2020: contributor

IIRC it’s trivial to get rid of by refactoring the loop into a do while… sorry, I noticed this months ago (its only in compilers I don’t usually use and only happens with endo enabled) and talked to sipa about it but neither of us opened a pull request. (I had a patch in a pastebin, but it expired…)

Are you talking about his loop? https://github.com/bitcoin-core/secp256k1/blob/c6b6b8f1bb044d7d1aa065ebb674adde98a36a8e/src/ecmult_impl.h#L495

It’s not clear to me do refactor this into a do while.

sipa commented at 6:43 pm on October 26, 2020: contributor

It’s not clear to me do refactor this into a do while.

This works for me:

 0diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h
 1index 057a69c..c474fc3 100644
 2--- a/src/ecmult_impl.h
 3+++ b/src/ecmult_impl.h
 4@@ -491,15 +491,14 @@ static void secp256k1_ecmult_strauss_wnaf(const secp256k1_ecmult_context *ctx, c
 5      */
 6     if (no > 0) {
 7         /* Compute the odd multiples in Jacobian form. */
 8-        secp256k1_ecmult_odd_multiples_table(ECMULT_TABLE_SIZE(WINDOW_A), state->prej, state->zr, &a[state->ps[0].input_pos]);
 9-        for (np = 1; np < no; ++np) {
10+        for (np = 0; np < no; ++np) {
11             secp256k1_gej tmp = a[state->ps[np].input_pos];
12 #ifdef VERIFY
13-            secp256k1_fe_normalize_var(&(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
14+            if (np) secp256k1_fe_normalize_var(&(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
15 #endif
16-            secp256k1_gej_rescale(&tmp, &(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
17+            if (np) secp256k1_gej_rescale(&tmp, &(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
18             secp256k1_ecmult_odd_multiples_table(ECMULT_TABLE_SIZE(WINDOW_A), state->prej + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &tmp);
19-            secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
20+            if (np) secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
21         }
22         /* Bring them to the same Z denominator. */
23         secp256k1_ge_globalz_set_table_gej(ECMULT_TABLE_SIZE(WINDOW_A) * no, state->pre_a, &Z, state->prej, state->zr);

#841 also silences the warning for me. Sorry for not pointing this out earlier, I had been seeing that since a few months, but forgot about it as it only triggered when the endomorphism was enabled.

hebasto commented at 8:34 pm on October 26, 2020: member

This works for me:

 0--- a/src/secp256k1/src/ecmult_impl.h
 1+++ b/src/secp256k1/src/ecmult_impl.h
 2@@ -491,15 +491,14 @@ static void secp256k1_ecmult_strauss_wnaf(const secp256k1_ecmult_context *ctx, c
 3      */
 4     if (no > 0) {
 5         /* Compute the odd multiples in Jacobian form. */
 6-        secp256k1_ecmult_odd_multiples_table(ECMULT_TABLE_SIZE(WINDOW_A), state->prej, state->zr, &a[state->ps[0].input_pos]);
 7-        for (np = 1; np < no; ++np) {
 8+        for (np = 0; np < no; ++np) {
 9             secp256k1_gej tmp = a[state->ps[np].input_pos];
10 #ifdef VERIFY
11-            secp256k1_fe_normalize_var(&(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
12+            if (np) secp256k1_fe_normalize_var(&(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
13 #endif
14-            secp256k1_gej_rescale(&tmp, &(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
15+            if (np) secp256k1_gej_rescale(&tmp, &(state->prej[(np - 1) * ECMULT_TABLE_SIZE(WINDOW_A) + ECMULT_TABLE_SIZE(WINDOW_A) - 1].z));
16             secp256k1_ecmult_odd_multiples_table(ECMULT_TABLE_SIZE(WINDOW_A), state->prej + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &tmp);
17-            secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
18+            if (np) secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
19         }
20         /* Bring them to the same Z denominator. */
21         secp256k1_ge_globalz_set_table_gej(ECMULT_TABLE_SIZE(WINDOW_A) * no, state->pre_a, &Z, state->prej, state->zr);

0$ make > /dev/null
1# no warnings :)

hebasto commented at 8:37 pm on October 26, 2020: member

@real-or-random

There’s a small chance that this is related to #839.

@hebasto Can you check if the warning disappears with this PR?

#839 seems actively developing now. Let’s wait for a bit more stable state, at least until the next morning in my tz :)

gmaxwell commented at 10:46 pm on October 26, 2020: contributor

uhg. please don’t litter the function with conditionals to suppress an outright spurious warning.

sipa commented at 11:08 pm on October 26, 2020: contributor

@gmaxwell I’m trying to remember what the “simple fix” is we had a few months ago, but I don’t think it works without the conditionals. In any case, #841 seems like a much better and simpler change.

gmaxwell commented at 11:19 pm on October 26, 2020: contributor

I thought it just had one (around the whole block), but perhaps not– perhaps I didn’t like it either and thats why it didn’t get PRed. :)

real-or-random referenced this in commit 8893f42438 on Oct 27, 2020

hebasto commented at 7:36 am on October 27, 2020: member

@real-or-random

There’s a small chance that this is related to #839.

@hebasto Can you check if the warning disappears with this PR?

Tested #839 (5fd2742d2117336e425c45ff3b29d7add09311af). Warnings are still emitted.

jonasnick closed this on Oct 27, 2020

sipa cross-referenced this on Oct 28, 2020 from issue Update secp256k1 subtree to latest master by sipa

fanquake referenced this in commit 8e9e190ea5 on Oct 29, 2020

sidhujag referenced this in commit fc69064dca on Oct 29, 2020

UdjinM6 referenced this in commit ee2a08fe89 on Aug 10, 2021

5tefan referenced this in commit 9ddb79318d on Aug 12, 2021

gcc emits -Warray-bounds warnings #834