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.
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 cd473e02c372217c3a6608ce5afaa543ed78f891
jonasnick:
utACK cd473e02c372217c3a6608ce5afaa543ed78f891
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
Most of the codebase correctly used short-cutting to avoid calling
_is_zero on possibly incompletely initialized elements, but a few
places were missed.
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.
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.
This changes pointer calculations in size comparions to a form that
ensures that no out-of-bound pointers are computed, because even their
computation yields undefined behavior.
Also, this changes size comparions to a form that ensures that neither
the left-hand side nor the right-hand side can overflow.
dbed75d Undefine `STATIC_PRECOMPUTATION` if using the basic config (DesWurstes)
310111e Keep LDFLAGS if `--coverage` (DesWurstes)
Pull request description:
Update: **This is a trimmed pull request with strong rationale.**
- Adding `--coverage` shouldn't reset `LDFLAGS`, this is definitely a typo
- The basic configuration should undefine `STATIC_PRECOMPUTATION`, as generating it is not supported and it complicates #549
Tree-SHA512: 29f0dd4c870ec60d535346446b453da459ca843ed1265c2bc966bf0fcbdf3c5c79f9e48a419662e81d790a7003f8877a16e2a5a74aa5c0b79645e15ad56a0f66
248f046 Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...) (practicalswift)
Pull request description:
Make sure we're not using an uninitialized variable in `secp256k1_wnaf_const(...)`:
```
In file included from src/secp256k1.c:15:0,
from src/tests.c:17:
src/ecmult_const_impl.h: In function ‘secp256k1_wnaf_const’:
src/ecmult_const_impl.h:117:20: warning: ‘u’ may be used uninitialized in this function [-Wmaybe-uninitialized]
wnaf[word] = u * global_sign;
^
```
**Note to reviewers:** Perhaps an `assert(…);` is a bit drastic. What would be a more graceful way to handle this? :-)
Tree-SHA512: 536cd7cc5b87a84fbaac578cecbba81b8d82e4672a30a2db9a674b82856132e79b0158a6a88609bc24942ebdbf1fcd2c4399a4c31ab0654b88ace9c0e6f1eaf3
8979ec0 Pass scalar by reference in secp256k1_wnaf_const() (Tim Ruffing)
Pull request description:
After this change, no struct or union is passed by value in the
entire codebase. This makes it easier to compile the library with
CompCert.
Tree-SHA512: 6b23e2b39701c3eeb6ae8c8d660cabe8872ac8f13141504c1ec55c47f2009e206129b34b31796e618114b60350598187df6df4c2be0e5c1b138a6126ad6a7484
The constraint "n" is a more obscure setting which is needed for
special cases that don't apply to our use.
Use of "n" exposed some bugs in pre-release versions of clang.
This fixes#594
