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.

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

Return NULL early in context_preallocated_create if flags invalid #840

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

real-or-random
Copy link
Contributor

If the user passes invalid flags to _context_create, and the default
illegal callback does not abort the program (which is possible), then we
work with the result of malloc(0), which may be undefined behavior. This
violates the promise that a library function won't crash after the
illegal callback has been called.

This commit fixes this issue by returning NULL early in _context_create
in that case.

Copy link
Contributor

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, but the newly introduced VERIFY_CHECK is wrong, causing tests to fail.

src/secp256k1.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Local tests pass with the != -> == fix.

If the user passes invalid flags to _context_create, and the default
illegal callback does not abort the program (which is possible), then we
work with the result of malloc(0), which may be undefined behavior. This
violates the promise that a library function won't crash after the
illegal callback has been called.

This commit fixes this issue by returning NULL early in _context_create
in that case.
@real-or-random
Copy link
Contributor Author

Fixed...

@sipa
Copy link
Contributor

sipa commented Oct 27, 2020

ACK ebfa205

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ebfa205

@gmaxwell
Copy link
Contributor

Should the header at secp256k1_preallocated.h indicate that this can return a null pointer on error? (I guess it could before too, but it appears to be undocumented.)

@real-or-random
Copy link
Contributor Author

Should the header at secp256k1_preallocated.h indicate that this can return a null pointer on error? (I guess it could before too, but it appears to be undocumented.)

The same is true for the the normal _context_create, which calls _context_preallocated_create (this is the case when we get malloc(0). This issue has also been brought up in #783. What is true is that we say the return values of the function are undefined if the illegal callback fires. And this is enough IMO.

But the docs are really difficult to understand here. We give a promise that the illegal callback "will only trigger for violations that are mentioned explicitly in the header." and this is not really mentioned (or at least) it's not very implicit. We may want to add a sentence for each function can calls the illegal callback but I don't know if we want to commit on this... I suggest to discuss this in #783.

@gmaxwell
Copy link
Contributor

K. Just thought I'd mention it.

@jonasnick jonasnick merged commit 903b16a into bitcoin-core:master Oct 30, 2020
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 31, 2020
… invalid

Summary:
```
If the user passes invalid flags to _context_create, and the default
illegal callback does not abort the program (which is possible), then we
work with the result of malloc(0), which may be undefined behavior. This
violates the promise that a library function won't crash after the
illegal callback has been called.

This commit fixes this issue by returning NULL early in _context_create
in that case.
```

Backport of secp256k1 [[bitcoin-core/secp256k1#840 | PR840]].

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8202
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Nov 1, 2020
… invalid

Summary:
```
If the user passes invalid flags to _context_create, and the default
illegal callback does not abort the program (which is possible), then we
work with the result of malloc(0), which may be undefined behavior. This
violates the promise that a library function won't crash after the
illegal callback has been called.

This commit fixes this issue by returning NULL early in _context_create
in that case.
```

Backport of secp256k1 [[bitcoin-core/secp256k1#840 | PR840]].

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants