Avoid optimizing out a verify_check #644

pull elichai wants to merge 1 commits into bitcoin-core:master from elichai:check changing 1 files +2 −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
real-or-random commented at 11:30 PM on July 2, 2019: contributor

ACK, I read the code
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.
jonasnick commented at 11:38 AM on July 3, 2019: contributor

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

ACK read the code
jonasnick approved
jonasnick commented at 8:39 PM on July 3, 2019: contributor

ACK read the code
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.
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?
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).
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 :)
elichai renamed this:
~~checking null pointer before dereferencing~~
Avoid optimizing out a verify_check
on Jul 5, 2019
sipa commented at 9:56 PM on August 6, 2019: contributor

ACK 94ae7cbf83a34456e5cad721f61ea77fcc023a3f
sipa merged this on Aug 6, 2019
sipa closed this on Aug 6, 2019
sipa referenced this in commit e95f8ab098 on Aug 6, 2019
sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa
fanquake referenced this in commit 8c97780db8 on Jun 13, 2020
sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020
ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020
UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021
5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021
gades referenced this in commit d855cc511d on May 8, 2022

Contributors

Linked (view graph)

#643 Null checks before dereferencing a pointer