Avoid optimizing out a verify_check #644

1. 
   elichai commented at 11:22 pm on July 2, 2019: contributor

   Before that even on debug the compiler could’ve assumed a isn’t null and optimized VERIFY_CHECK(a != NULL); out. This put the dereference after the check Resolves #643
2. 
   real-or-random commented at 11:30 pm on July 2, 2019: contributor

   ACK, I read the code
3. in src/ecmult_impl.h:408 in 828ffd696d outdated

   404@@ -405,6 +405,7 @@ static int secp256k1_ecmult_wnaf(int *wnaf, int len, const secp256k1_scalar *a,
   405     VERIFY_CHECK(0 <= len && len <= 256);
   406     VERIFY_CHECK(a != NULL);
   407     VERIFY_CHECK(2 <= w && w <= 31);
   408+    s = *a;
   

   ---

   ---

   

   jonasnick commented at 11:37 am on July 3, 2019:

   nit: this seems like a pretty random location for this line. How about putting it right above the first usage of s?

   ---

   

   elichai commented at 1:40 pm on July 3, 2019:

   I tried to make the change make sense by putting it right after the checks but you’re right, in the future this should make sense without knowing there was some change. i’ll fix.
4. 
   jonasnick commented at 11:38 am on July 3, 2019: contributor

   ACK mod nit
5. Moved a dereference so the null check will be before the dereferencing 94ae7cbf83
6. elichai force-pushed on Jul 3, 2019
7. real-or-random approved
8. 
   real-or-random commented at 2:44 pm on July 3, 2019: contributor

   ACK read the code
9. jonasnick approved
10. 
    jonasnick commented at 8:39 pm on July 3, 2019: contributor

    ACK read the code
11. 
    gmaxwell commented at 1:49 am on July 5, 2019: contributor

    This description is one which would incorrectly cause a overblown reaction. I would have described it as “prevent the compiler from optimizing out an interface invariant assertion” or similar. “checking null pointer before dereferencing” is a description people use when fixing serious vulnerabilities.
12. 
    elichai commented at 1:55 am on July 5, 2019: contributor

    When I named this I tried to think of a name that won’t sound alarming but still describe what’s the point. I didn’t try to make this sound like a “vulnerability”. it’s not. It’s so nothing I wasn’t even sure if it’s worth a PR and that’s why I opened an issue first #643

    If anyone looking at this PR thought it is a vulnerability it’s my fault. suggestions for a title?

13. 
    gmaxwell commented at 2:02 am on July 5, 2019: contributor

    No need to apologize, you did nothing wrong. We want people to treat serious issues seriously, but an unfortunate side effect is that a little extra effort needs to go into not falsely triggering serious issue detection. My comment had a suggestion, “avoid optimizing out a verify_check” which was actually your goal! :) Generally it’s most informative if changes are described “why” first either describe the “how” later (or just leave it to the code itself to describe the “how”).

    To add some additional colour which might be useful to you in the future there is actually a fair amount of machinery in the codebase already just to avoid optimizing out VERIFY_CHECKs (and even some ARG_CHECKs): Most of the external interface is annotated with nonnull in order to cause the compiler to warn at compile time on the caller’s code when it can prove that an argument will be null. Unfortunately that annotation is treated like a dereference when compiling the function and will cause null checks to be treated like dead code. As a result, when compiling the library itself those annotations are left out (triggered by SECP256K1_BUILD define).

14. 
    elichai commented at 2:09 am on July 5, 2019: contributor

    Yeah i’m still trying to figure out all the autotools and macro configurations in this library :)
15. elichai renamed this:
    checking null pointer before dereferencing
    Avoid optimizing out a verify_check
    on Jul 5, 2019
16. 
    sipa commented at 9:56 pm on August 6, 2019: contributor

    ACK 94ae7cbf83a34456e5cad721f61ea77fcc023a3f
17. sipa merged this on Aug 6, 2019
18. sipa closed this on Aug 6, 2019

    ---

19. sipa referenced this in commit e95f8ab098 on Aug 6, 2019
20. sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa
21. fanquake referenced this in commit 8c97780db8 on Jun 13, 2020
22. sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020
23. ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020
24. UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021
25. 5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021
26. gades referenced this in commit d855cc511d on May 8, 2022

Contributors
elichai real-or-random jonasnick gmaxwell sipa