Null checks before dereferencing a pointer #643

1. 
   elichai commented at 8:30 pm on July 2, 2019: contributor

   Hi, As far as I could see this doesn’t currently affect anything in the code but there’s a pointer dereference here: https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_impl.h#L398

   and only a few lines afterwards we check if it’s not null https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_impl.h#L406

2. 
   elichai commented at 9:19 pm on July 2, 2019: contributor

   It seems that VERIFY_CHECK is for debug only,and if so this doesn’t matter too much .

   (haven’t yet grasped all of the different checks here and what each one does under different flags)

3. 
   real-or-random commented at 11:00 pm on July 2, 2019: contributor

   Good catch. If we check it, then we should check it before we dereference it… (Otherwise the compiler may just remove the check.)

   Do you want to open a PR?

   Different checks:

   • CHECK is always enabled, and calls the error callback on failure.
   • VERIFY_CHECK is the same but only enabled in debug
   • ARG_CHECK is always enabled, and calls the illegal (argument) callback.
   • ARG_CHECK_NO_RETURN is like ARG_CHECK but for use in void functions

   For the usage see #566 (comment), it’s not 100% consistent throughout the library, and there are cases where it’s not 100% clear what the right check is. As I interpret this guideline, it will acceptable to replace the VERIFY_CHECK by a CHECK here. But for the sake of doing only a minimal change, I recommend leaving it as it is and not caring too much about it.

4. elichai cross-referenced this on Jul 2, 2019 from issue Avoid optimizing out a verify_check by elichai
5. 
   elichai commented at 11:22 pm on July 2, 2019: contributor

   Sure, opened a PR.
6. sipa closed this on Aug 6, 2019

   ---

7. sipa referenced this in commit e95f8ab098 on Aug 6, 2019

Contributors
elichai real-or-random