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

tests: Use BasicTestingSetup to initialise fuzzing environment #19074

Closed

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented May 26, 2020

Use BasicTestingSetup to initialise fuzzing environment (as suggested by MarcoFalke).

Also: Remove unreachable fuzzing code. Various small coverage improvements.

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

@DrahtBot DrahtBot added the Tests label May 26, 2020
@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 2, 2020

Added commit: "tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests".

@maflcko
Copy link
Member

maflcko commented Jun 2, 2020

That commit seems unrelated. Mind splitting it up so that the libevent reviewers can review it?

@practicalswift
Copy link
Contributor Author

@MarcoFalke Done!

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 9, 2020

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

Conflicts

No conflicts as of last run.

maflcko pushed a commit that referenced this pull request Jul 10, 2020
…event when handling PROXY requests

20d31bd tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests (practicalswift)

Pull request description:

  Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering a `nullptr` dereference. Split out from #19074 as suggested by MarcoFalke.

  The dereference (`req->evcon->http_server`) takes place in `evhttp_parse_request_line` and is a consequence of our hacky but necessary use of the internal function `evhttp_parse_firstline_` in the `http_request` fuzzing harness.

  The suggested workaround is not aesthetically pleasing, but it successfully avoids the troublesome code path.

  `" http:// HTTP/1.1\n"` was a crashing input prior to this workaround.

  Before this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==27905==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x55a169b7e053 bp 0x7ffd452f1160 sp 0x7ffd452f10e0 T0)
  ==27905==The signal is caused by a READ memory access.
  ==27905==Hint: address points to the zero page.
      #0 0x55a169b7e053 in evhttp_parse_request_line depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:1883:37
      #1 0x55a169b7d9ae in evhttp_parse_firstline_ depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:2041:7
      #2 0x55a1687f624e in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/http_request.cpp:51:9
  …
  $ echo $?
  1
  ```

  After this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  Executed input in 0 ms
  ***
  *** NOTE: fuzzing was not performed, you have only
  ***       executed the target code on a fixed set of inputs.
  ***
  $ echo $?
  0
  ```

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

Top commit has no ACKs.

Tree-SHA512: 7a6b68e52cbcd6c117487e74e47760fe03566bec09b0bb606afb3b652edfd22186ab8244e8e27c38cef3fd0d4a6c237fe68b2fd22e0970c349e4ab370cf3e304
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 11, 2020
… in libevent when handling PROXY requests

20d31bd tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests (practicalswift)

Pull request description:

  Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering a `nullptr` dereference. Split out from bitcoin#19074 as suggested by MarcoFalke.

  The dereference (`req->evcon->http_server`) takes place in `evhttp_parse_request_line` and is a consequence of our hacky but necessary use of the internal function `evhttp_parse_firstline_` in the `http_request` fuzzing harness.

  The suggested workaround is not aesthetically pleasing, but it successfully avoids the troublesome code path.

  `" http:// HTTP/1.1\n"` was a crashing input prior to this workaround.

  Before this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==27905==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x55a169b7e053 bp 0x7ffd452f1160 sp 0x7ffd452f10e0 T0)
  ==27905==The signal is caused by a READ memory access.
  ==27905==Hint: address points to the zero page.
      #0 0x55a169b7e053 in evhttp_parse_request_line depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:1883:37
      #1 0x55a169b7d9ae in evhttp_parse_firstline_ depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:2041:7
      #2 0x55a1687f624e in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/http_request.cpp:51:9
  …
  $ echo $?
  1
  ```

  After this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  Executed input in 0 ms
  ***
  *** NOTE: fuzzing was not performed, you have only
  ***       executed the target code on a fixed set of inputs.
  ***
  $ echo $?
  0
  ```

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

Top commit has no ACKs.

Tree-SHA512: 7a6b68e52cbcd6c117487e74e47760fe03566bec09b0bb606afb3b652edfd22186ab8244e8e27c38cef3fd0d4a6c237fe68b2fd22e0970c349e4ab370cf3e304
@practicalswift
Copy link
Contributor Author

@MarcoFalke I think you requested something along the lines of this PR at one time: do we want to proceed with this or should I close? :)

@practicalswift practicalswift deleted the fuzzers-2020-05-26 branch April 10, 2021 19:42
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 17, 2022
… in libevent when handling PROXY requests

20d31bd tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests (practicalswift)

Pull request description:

  Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering a `nullptr` dereference. Split out from bitcoin#19074 as suggested by MarcoFalke.

  The dereference (`req->evcon->http_server`) takes place in `evhttp_parse_request_line` and is a consequence of our hacky but necessary use of the internal function `evhttp_parse_firstline_` in the `http_request` fuzzing harness.

  The suggested workaround is not aesthetically pleasing, but it successfully avoids the troublesome code path.

  `" http:// HTTP/1.1\n"` was a crashing input prior to this workaround.

  Before this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==27905==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x55a169b7e053 bp 0x7ffd452f1160 sp 0x7ffd452f10e0 T0)
  ==27905==The signal is caused by a READ memory access.
  ==27905==Hint: address points to the zero page.
      #0 0x55a169b7e053 in evhttp_parse_request_line depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:1883:37
      #1 0x55a169b7d9ae in evhttp_parse_firstline_ depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:2041:7
      #2 0x55a1687f624e in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/http_request.cpp:51:9
  …
  $ echo $?
  1
  ```

  After this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  Executed input in 0 ms
  ***
  *** NOTE: fuzzing was not performed, you have only
  ***       executed the target code on a fixed set of inputs.
  ***
  $ echo $?
  0
  ```

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

Top commit has no ACKs.

Tree-SHA512: 7a6b68e52cbcd6c117487e74e47760fe03566bec09b0bb606afb3b652edfd22186ab8244e8e27c38cef3fd0d4a6c237fe68b2fd22e0970c349e4ab370cf3e304
@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.

3 participants