Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...) #533

Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...):

In file included from src/secp256k1.c:15:0,
                 from src/tests.c:17:
src/ecmult_const_impl.h: In function ‘secp256k1_wnaf_const’:
src/ecmult_const_impl.h:117:20: warning: ‘u’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     wnaf[word] = u * global_sign;
                    ^

Note to reviewers: Perhaps an assert(…); is a bit drastic. What would be a more graceful way to handle this? :-)

We don't use assert in the secp256k1 codebase. Instead I suggest you:

• Turn the while loop into a do loop.
• Add a VERIFY_CHECK(size > 0); (which only affects the code in unit tests).

@sipa Makes perfect sense. Thanks for the quick feedback!

I've now pushed an updated version. Please re-review :-)

utACK 5f10203

ACK

; ) 確認

utACK

On re-review, I believe we need to VERIFY_CHECK w>0 for the loop transformation to be correct. (obviously w>0 or the code is already broken... so.) @sipa opinion?

I think it's best to just VERIFY_CHECK(w > 0) too. If w == 0, we even divide by 0 in the VERIFY_CHECK here: https://github.com/bitcoin-core/secp256k1/blob/5f10203cfe3c40e7295aced2f36e090bb3f50e33/src/ecmult_const_impl.h#L119

@practicalswift: Could you update this?

@real-or-random Sure!

Just to clarify (better safe than sorry!):

Is the suggestion to change from:

VERIFY_CHECK(size > 0);

To:

VERIFY_CHECK(w > 0);
VERIFY_CHECK(size > 0);

And otherwise leave the PR unchanged?

Yes that's my suggestion. :)

@real-or-random Updated. Please re-review :-)

utACK 248f0466112c96b9851c662fa829f20d28d16344

ACK