Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Null checks before dereferencing a pointer #643

Closed
elichai opened this issue Jul 2, 2019 · 3 comments · Fixed by #644
Closed

Null checks before dereferencing a pointer #643

elichai opened this issue Jul 2, 2019 · 3 comments · Fixed by #644

Comments

@elichai
Copy link
Contributor

elichai commented Jul 2, 2019

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
@elichai
Copy link
Contributor Author

elichai commented Jul 2, 2019

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)

@real-or-random
Copy link
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.

@elichai elichai mentioned this issue Jul 2, 2019
Merged
@elichai
Copy link
Contributor Author

elichai commented Jul 2, 2019

Sure, opened a PR.

@sipa sipa closed this as completed in #644 Aug 6, 2019
sipa added a commit that referenced this issue Aug 6, 2019
@sipa
Merge #644: Avoid optimizing out a verify_check
e95f8ab 
94ae7cb Moved a dereference so the null check will be before the dereferencing (Elichai Turkel)

Pull request description:

  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

ACKs for commit 94ae7c:
  sipa:
    ACK 94ae7cb

Tree-SHA512: 8b986f202ede5bde1f14a8ecf25e339d64ee6cd5cb391c5f18b4ff58f946c3845902d1230bc80d110a0a33b37025d281bd4532afbdf03b1c9ca321097374eb8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid optimizing out a verify_check elichai/secp256k1
2 participants
@real-or-random @elichai