Skip to content

Conversation

@practicalswift
Copy link
Contributor

Skip unnecessary fuzzer initialisation. Hold ECCVerifyHandle only when needed.

As suggested by MarcoFalke in #17018 (comment).

@practicalswift practicalswift force-pushed the fuzz-initialize-when-needed branch from c6adfa5 to fd17c64 Compare October 23, 2019 21:56
@fanquake fanquake added the Tests label Oct 23, 2019
@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17225 (tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. by practicalswift)
  • #17109 (tests: Add fuzzing harness for various functions consuming only integrals by practicalswift)
  • #17071 (tests: Add fuzzing harness for CheckBlock(...) and other CBlock related functions by practicalswift)
  • #17051 (tests: Add deserialization fuzzing harnesses by practicalswift)
  • #16975 (test: Show debug log on unit test failure by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@practicalswift practicalswift force-pushed the fuzz-initialize-when-needed branch from fd17c64 to c2f964a Compare October 24, 2019 08:09
maflcko pushed a commit that referenced this pull request Oct 24, 2019
…VerifyHandle only when needed.

c2f964a tests: Remove Cygwin WinMain workaround (practicalswift)
db4bd32 tests: Skip unnecessary fuzzer initialisation. Hold ECCVerifyHandle only when needed. (practicalswift)

Pull request description:

  Skip unnecessary fuzzer initialisation. Hold `ECCVerifyHandle` only when needed.

  As suggested by MarcoFalke in #17018 (comment).

Top commit has no ACKs.

Tree-SHA512: 598da44859d736e3fdc143b93e07f444d8ad19dfdab0cfe7c6ccff8644e862664d869337dfe6b49416ed09a0024e4a5f2220ca6246de568f9e9227d721baa28e
@maflcko maflcko merged commit c2f964a into bitcoin:master Oct 24, 2019
maflcko pushed a commit that referenced this pull request Oct 31, 2019
…dding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to #17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 31, 2019
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 10, 2020
…rifyHandle only when needed.

Summary:
This diff squashes three Core PRs into one. The reason is that [[bitcoin/bitcoin#17235 | PR17235]] introduces a bug, and [[bitcoin/bitcoin#17274 | PR17274]] and [[bitcoin/bitcoin#17685 | PR17685]] both fix it, so our fuzzing test setup isn't broken at any point.

---

c2f964a6745be085f2891c909d6c998687de9080 tests: Remove Cygwin WinMain workaround (practicalswift)
db4bd32cc31789fc017f5db0b86a69ee43e41575 tests: Skip unnecessary fuzzer initialisation. Hold ECCVerifyHandle only when needed. (practicalswift)

Pull request description:

  Skip unnecessary fuzzer initialisation. Hold `ECCVerifyHandle` only when needed.

  As suggested by MarcoFalke in bitcoin/bitcoin#17018 (comment).

---

Merge #17274: tests: Fix fuzzers eval_script and script_flags by re-adding ECCVerifyHandle dependency

9cae3d5e94f4481e0d251c924314e57187a07a60 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to #17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

---

Merge #17685: tests: Fix bug in the descriptor parsing fuzzing harness (descriptor_parse)

6338c0203416a5f86e9422b6cd479da8af277f2f tests: Fix fuzzing harness for descriptor parsing (descriptor_parse) (practicalswift)

Pull request description:

  Fix bug in the descriptor parsing fuzzing harness (`descriptor_parse`) by making sure `secp256k1_context_verify` is properly initialized (via `ECCVerifyHandle`).

  Background:

  When fuzzing `Parse(…)` with `libFuzzer` I eventually reached the test case `combo(020000000000000000000000000000000000000000000000000000000000000000)`. That input triggers a call to `CPubKey::IsFullyValid()` which in turns requires an initialized `secp256k1_context_verify`.

  The fuzzing harness did not fulfil that pre-condition prior to this commit (sorry, my fault!) :)

---

Depends on D6881

Backport of Core [[bitcoin/bitcoin#17235 | PR17235]], [[bitcoin/bitcoin#17274 | PR17274]] and [[bitcoin/bitcoin#17685 | PR17685]]

Test Plan:
  cmake -GNinja .. -DENABLE_SANITIZERS="address;fuzzer" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
  ninja bitcoin-fuzzers link-fuzz-test_runner.py
  ./test/fuzz/test-runner.py -l DEBUG <path-to-corpus>

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6883
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
@practicalswift practicalswift deleted the fuzz-initialize-when-needed branch April 10, 2021 19:39
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 6, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 11, 2021
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 7, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants