This pull request fixes a bug which allows the `sh` variable to be used uninitialized when sscanf returns EOF.
Signed-off-by: Mustapha Abiola <mustapha@trilemma.net>
Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour.
While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands.
6914c25 typo in comment for secp256k1_ec_pubkey_tweak_mul () (philsmd)
Pull request description:
Fixes a typo in secp256k1.h documentation
ACKs for commit 6914c2:
Tree-SHA512: 9b95209b7decab4624054b5e3476e99468f84f84eb270bba997abf73a78acbbf2eaa094dfa367ebfe0b1e553329071e9a0ca8a1e2b31ea7fbc4aad3fb0665e88
cd473e0 Avoid calling secp256k1_*_is_zero when secp256k1_*_set_b32 fails. (Gregory Maxwell)
Pull request description:
Most of the codebase correctly used short-cutting to avoid calling
_is_zero on possibly incompletely initialized elements, but a few
places were missed.
ACKs for commit cd473e:
sipa:
utACK cd473e02c3
jonasnick:
utACK cd473e02c3
Tree-SHA512: d6af2863f6795d2df26f2bd05a4e33085e88c45f7794601ea57e67238a2073ef1ee3ba0feab62a7fcbc0636c48dfd80eea07d0ca4f194414127f914b0478c732
dcf3920 Fix ability to compile tests without -DVERIFY. (Gregory Maxwell)
Pull request description:
Broken by 3f3964e4.
It's important that the tests are also run without -DVERIFY due to
the possibility that side-effects of a VERIFY_CHECK fix a bug that
would otherwise be detected.
Use of the verify_check macro in tests isn't sufficient.
ACKs for commit dcf392:
Tree-SHA512: ff7ca0e89e33f845656a4d7d18c0195d1378b020d67f89e900b18cf3d702aa81dd91ffd05a98953a481b83e4247eaf0c484bea12eab020efb3c966a456e8129f
14c7dbd Simplify control flow in DER parsing (Tim Ruffing)
ec8f20b Avoid out-of-bound pointers and integer overflows in size comparisons (Tim Ruffing)
01ee1b3 Parse DER-enconded length into a size_t instead of an int (Tim Ruffing)
3cb057f Fix possible integer overflow in DER parsing (Tim Ruffing)
Pull request description:
This is a result of auditing the code for overflow issues at random places. None of this is critical but I think all of it should be fixed.
I know this touches "red" code. I double-checked and triple-checked this but I can understand if some of the changes are not desirable because they change well-tested code.
Best reviewed in individual commits.
ACKs for commit 14c7db:
Tree-SHA512: 312dd3f961739752e1a861e75bd755920f634f87ee9668793e102c224434e8d21367452e114de729322c71a89f4fa82126aa5d32742f2bbbc091777c99515e10
e49f799 Add missing #(un)defines to base-config.h (Tim Ruffing)
77defd2 Add secp256k1_ prefix to default callback functions (Tim Ruffing)
908bdce Include stdio.h and stdlib.h explicitly in secp256k1.c (Tim Ruffing)
5db782e Allow usage of external default callbacks (Tim Ruffing)
6095a86 Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return (Tim Ruffing)
Pull request description:
This is intended for environments without implementations for `abort()`, `fprintf()`, and `stderr`. e.g., embedded systems. Those can provide their own implementations of `default_illegal_callback_fn` and `default_error_callback_fn` at compile time.
If you want to use your own default callback, things will be somewhat inconsistent unfortunately: We cannot make the callback data `extern` too, because then the initialization lists for `default_illegal_callback` won't contain only constants. (`const` variables are not compile-time constants). So you cannot take callback data in your own default callback function.
As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.
Note that `abort()`, `fprintf()`, and `stderr` are also used in `CHECK`, which is still used in production code if we rely on gmp for scalar and field inversions (e.g., https://github.com/bitcoin-core/secp256k1/blob/master/src/scalar_impl.h#L240). This is not an issue for embedded system which probably don't want to use gmp anyway, but it is probably an issue for the reasons explained in https://github.com/bitcoin-core/secp256k1/pull/566#issuecomment-469111901.
(related downstream: https://github.com/rust-bitcoin/rust-secp256k1/pull/100 @elichai)
ACKs for commit e49f79:
Tree-SHA512: 4dec0821eef4156cbe162bd8cdf0531c1fae8c98cd9db8438170ff1aa0e59b199739eeab293695bb582246812bea5309959f02f1fb74bb57872da54ebc52313f
Most of the codebase correctly used short-cutting to avoid calling
_is_zero on possibly incompletely initialized elements, but a few
places were missed.
98836b1 scratch: replace frames with "checkpoint" system (Andrew Poelstra)
7623cf2 scratch: save a couple bytes of unnecessarily-allocated memory (Andrew Poelstra)
a7a164f scratch: rename `max_size` to `size`, document that extra will actually be allocated (Andrew Poelstra)
5a4bc0b scratch: unify allocations (Andrew Poelstra)
c2b028a scratch space: thread `error_callback` into all scratch space functions (Andrew Poelstra)
0be1a4a scratch: add magic bytes to beginning of structure (Andrew Poelstra)
92a48a7 scratch space: use single allocation (Andrew Poelstra)
Pull request description:
ACKs for commit 98836b:
Tree-SHA512: 6e251f704644a5f61b24aa05c6f7a31ad8c58d147195079d52fe45daacd28a9fd2f4aaf71273183b99b3795a01a88f8389170d4280489b2a28a14a56e03153d7
9ab96f7 Use trivial algorithm in ecmult_multi if scratch space is small (Jonas Nick)
Pull request description:
`ecmult_multi` already selects the trivial algorithm if the scratch space is NULL. With this PR the trivial algorithm is also selected if the scratch space is too small to use pippenger or strauss instead of returning 0. That makes it more easier to avoid consensus relevant inconsistencies just because scratch space construction was messed up.
ACKs for commit 9ab96f:
real-or-random:
utACK 9ab96f7
Tree-SHA512: aa451adf8880af15cf167a59cb07fc411edc43f26c8eb0873bdae2774382ba182e2a1c54487912f8f2999cb0402d554b9d293e2fb9483234471348a1f43c6653
Broken by 3f3964e4.
It's important that the tests are also run without -DVERIFY due to
the possibility that side-effects of a VERIFY_CHECK fix a bug that
would otherwise be detected.
Use of the verify_check macro in tests isn't sufficient.
0522caa Explain caller's obligations for preallocated memory (Tim Ruffing)
238305f Move _preallocated functions to separate header (Tim Ruffing)
695feb6 Export _preallocated functions (Tim Ruffing)
814cc78 Add tests for contexts in preallocated memory (Tim Ruffing)
ba12dd0 Check arguments of _preallocated functions (Tim Ruffing)
5feadde Support cloning a context into preallocated memory (Tim Ruffing)
c4fd5da Switch to a single malloc call (Tim Ruffing)
ef020de Add size constants for preallocated memory (Tim Ruffing)
1bf7c05 Prepare for manual memory management in preallocated memory (Tim Ruffing)
Pull request description:
@apoelstra
This builds on #557.
Manually managing memory is always a pain in the ass in some way. I tried to keep the pain manageable. I'm open to suggestions to make this less ugly or error-prone.
to do:
* tests
* export functions
ACKs for commit 0522ca:
Tree-SHA512: 8ddb5b70219b6f095e780a9812d2387ab2a7f399803ce4101e27da504b479a61ebe08b6380568c7ba6f1e73d7d0b1f58a3c0a66fa0fdec7a64cd0740e156ce38
a61a93f Clean up ./configure help strings (Tim Ruffing)
2842dc5 Make WINDOW_G configurable (Tim Ruffing)
Pull request description:
This makes WINDOW_G a configurable value in the range of [2..24].
The upper limit of 24 is a defensive choice. The code is probably
correct for values up to 33 but those larger values yield in huge
tables (>= 256MiB), which are i) unlikely to be really beneficial
in practice and ii) increasingly difficult to test.
The main point of this is not to make the window size configurable (using ./configure) but rather to use an external #define for the window size, which makes it configurable for embedded system that rely on their own build system (like in #595).
ACKs for commit a61a93:
Tree-SHA512: 0d58fdf4763340ddab992e95f6302a33d891476a7ac1748202ee99808e72b20754bb6935cbeaf0bb36077abaaff7d65f4848b1af64f1a0a5258239ba0d27020c
This makes WINDOW_G a configurable value in the range of [2..24].
The upper limit of 24 is a defensive choice. The code is probably
correct for values up to 27 but those larger values yield in huge
tables (>= 256MiB), which are i) unlikely to be really beneficial
in practice and ii) increasingly difficult to test.
662918c Revert "Merge #620: Install headers automatically" (ian)
Pull request description:
This reverts commit 91fae3ace0, reversing
changes made to 5df77a0eda.
See discussion in https://github.com/bitcoin-core/secp256k1/pull/625
After the change, if we enable any module, `make install` fails because of the
duplicated files in the command line arguments.
Closes https://github.com/bitcoin-core/secp256k1/issues/624
Tree-SHA512: 7769ede527ac307bff094603e5974c56b19e41bc2ef55113173d3dbc8e545d6add7ae044753fa0361595e5e7a746d6c8d641f98caa3381b683aa2b61a1742097
Marko Bencun
Jan Xie
Mustapha Abiola
roconnor-blockstream
Elichai Turkel
Gregory Maxwell
philsmd
Tim Ruffing
Andrew Poelstra
ian