p2p: Refactor sock to add I2P fuzz and unit tests #21387
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
Concept ACK |
vasild
commented
Mar 8, 2021
src/netbase.cpp
Outdated
Comment on lines
-645
to
-575
Contributor
Author
There was a problem hiding this comment.
Note to reviewers: this old code was inconsistent with the events it requested depending on which function was used - it requested "in" or "out" (POLLIN | POLLOUT) when using poll() but only "out" when using select() (did not pass fdset as 2'nd arg to select()).
This is now replaced by Sock::Wait(..., requested Sock::RECV | Sock::SEND) which, if ends up calling select(), will ask it for both "in" and "out".
I think this is ok.
Member
There was a problem hiding this comment.
it might be good to split the non-fuzz "refactors" out into their own pull
Contributor
Author
There was a problem hiding this comment.
Lets see which are the non-fuzz refactors:
[7] i2p: add fuzz tests on the I2P Session public interface
[6] i2p: use pointers to Sock to accommodate mocking
[5] net: change ConnectSocketDirectly() to take a Sock argument
[4] net: add connect() and getsockopt() wrappers to Sock
[3] fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN
[2] fuzz: extend FuzzedSock::Recv() to support MSG_PEEK
[1] fuzz: implement unimplemented FuzzedSock methods
1-3 are obviously fuzz. 4 touches FuzzedSocket. 7 adds fuzz tests. So only 5 and 6 remain. 5 depends on the previous commits. 6 is non-fuzz, does not depend on the previous commits and can be extracted, but 7 depends on it.
So a split can be:
PR-A: 6
PR-B: 1-5, 7
I think it is not worth to split, given that 6 is a small mechanical change.
maflcko
reviewed
Mar 8, 2021
src/netbase.cpp
Outdated
Comment on lines
-645
to
-575
Member
There was a problem hiding this comment.
it might be good to split the non-fuzz "refactors" out into their own pull
vasild
commented
Mar 8, 2021
Contributor
|
Concept ACK Thanks for improving Very happy to see the I2P code being thoroughly fuzzed before landing in a release! "Fuzz before release" is a nice high bar I think we should try to aim for going forward for new features :) |
jonatack
reviewed
Mar 8, 2021
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Contributor
Author
|
|
Member
|
Saw this OOM error three times, but only with fuzz output
|
Member
|
Had the same OOM issue today with another fuzz PR, so I'm proceeding on the assumption that it's a regression that is not related to this PR. |
jonatack
reviewed
Mar 11, 2021
Member
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Built and reviewed each commit up to 34c199db11b36eee91e56cdd046c090f62da18d6.
In the 3rd commit, "avoid repeated errors," it looks like the commit message should be "to retry the operation." e.g. s/to/the/
Contributor
Author
Can you reproduce it? E.g. |
Member
|
jonatack
reviewed
Mar 12, 2021
Contributor
Author
|
|
Contributor
Author
|
|
Member
|
re-ACK 23c861d6ff995b7e6034d4bec6af2f6a3d595dca |
We want `Get()` to always return the same value, otherwise it will look like the `FuzzedSock` implementation itself is broken. So assign `m_socket` a random number in the `FuzzedSock` constructor. There is nothing to fuzz about the `Get()` and `Release()` methods, so use the ones from the base class `Sock`. `Reset()` is just setting our socket to `INVALID_SOCKET`. We don't want to use the base `Reset()` because it will close `m_socket` and given that our `m_socket` is just a random number it may end up closing a real opened file descriptor if it coincides with our random `m_socket`.
A conforming `recv(2)` call is supposed to return the same data on a call following `recv(..., MSG_PEEK)`. Extend `FuzzedSock::Recv()` to do that. For simplicity we only return 1 byte when `MSG_PEEK` is used. If we would return a buffer of N bytes, then we would have to keep track how many of them were consumed on subsequent non-`MSG_PEEK` calls.
Contributor
maflcko
changed the title
Member
|
(Changed title, because this is not test-only) |
Member
|
Concept ACK 40316a3 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Mar 30, 2021
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Feb 15, 2022
Summary: > net: add connect() and getsockopt() wrappers to Sock > > Extend the `Sock` class with wrappers to `connect()` and `getsockopt()`. > > This will make it possible to mock code which uses those. bitcoin/bitcoin@b586110 > net: change ConnectSocketDirectly() to take a Sock argument > > Change `ConnectSocketDirectly()` to take a `Sock` argument instead of a > bare `SOCKET`. With this, use the `Sock`'s (possibly mocked) methods > `Connect()`, `Wait()` and `GetSockOpt()` instead of calling the OS > functions directly. bitcoin/bitcoin@82d360b > i2p: use pointers to Sock to accommodate mocking > > Change the types of `i2p::Connection::sock` and > `i2p::sam::Session::m_control_sock` from `Sock` to > `std::unique_ptr<Sock>`. > > Using pointers would allow us to sneak `FuzzedSock` instead of `Sock` > and have the methods of the former called. > > After this change a test only needs to replace `CreateSock()` with > a function that returns `FuzzedSock`. bitcoin/bitcoin@9947e44 > test: add I2P test for a runaway SAM proxy > > Add a regression test for bitcoin/bitcoin#21407. > > The test creates a socket that, upon read, returns some data, but never > the expected terminator `\n`, injects that socket into the I2P code and > expects `i2p::sam::Session::Connect()` to fail, printing a specific > error message to the log. bitcoin/bitcoin@40316a3 This is a partial backport of [[bitcoin/bitcoin#21387 | core#21387]], without the fuzzer changes. Depends on D11037 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11042
maflcko
reviewed
Jun 9, 2022
|
|
||
| if (sess.Connect(to, conn, proxy_error)) { | ||
| try { | ||
| conn.sock->SendComplete("verack\n", 10ms, interrupt); |
Member
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment.
Well, it would be executed, but for Connect() to return true the fuzzed sock must return some meaningful data, which I guess has a very very low chance of happening.
Edit: now I remember I was considering crafting a custom fuzz-corpus data input which would contain the meaningful data at the right offsets so that Connect() will succeed. I gave up because that would be difficult to maintain because the binary "magic" corpus data would have to be adjusted every time this test is modified to add or remove calls that consume fuzz data before Connect() is called. Also, that would not be fuzzing so much, but rather some kind of unit testing.
Member
There was a problem hiding this comment.
There'd also be a middle way to teach the fuzz engine a meaningful format or snippets. This can be achieved with a dict, a custom mutator, or with some code in the fuzz target (CallOneOf(...))
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change the networking code and the I2P code to be fully mockable and use
FuzzedSocketto fuzz the I2P methodsListen(),Accept()andConnect().Add a mocked
Sockimplementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in #21407.