01960c53c7d71c70792abe19413315768dc2275a fuzz: make FuzzedDataProvider usage deterministic (Martin Leitner-Ankerl)
Pull request description:
There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
Unfortunately, [the order of evaluation of function arguments is unspecified](https://en.cppreference.com/w/cpp/language/eval_order), and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY
When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecified evaluation order could be a problem.
Finding these has been manual work; I grepped the sourcecode for these patterns, and looked at each usage individually. So there is a chance I missed some.
* `fuzzed_data_provider`
* `.Consume`
* `>Consume`
* `.rand`
I first discovered this in https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420236394. Note that there is a possibility that due to this fix the evaluation order is now different in many cases than when the fuzzing corpus has been created. If that is the case, the fuzzing corpus will have worse coverage than before.
Update: In list-initialization the order of evaluation is well defined, so e.g. usages in `initializer_list` or constructors that use `{...}` is ok.
ACKs for top commit:
achow101:
ACK 01960c53c7d71c70792abe19413315768dc2275a
vasild:
ACK 01960c53c7d71c70792abe19413315768dc2275a
ismaelsadeeq:
ACK 01960c53c7d71c70792abe19413315768dc2275a
Tree-SHA512: e56d087f6f4bf79c90b972a5f0c6908d1784b3cfbb8130b6b450d5ca7d116c5a791df506b869a23bce930b2a6977558e1fb5115bb4e061969cc40f568077a1ad
The script building logic performs a quadratic number of copies in the
number of nested wrappers in the miniscript. Limit the number of nested
wrappers to avoid fuzz timeouts.
Thanks to Marco Falke for reporting the fuzz timeouts and providing a
minimal input to reproduce.
This target may call into logic quadratic over the number of
sub-fragments. Limit the number of sub-fragments to keep the runtime
reasonable.
Thanks to Marco Falke for reporting the fuzz timeouts with a minimized
input.
Problem:
If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
retrieved from the fuzz provider, saved in `m_peek_data` and returned
to the caller (ok).
If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
then the first `M` bytes from `m_peek_data` would be returned
to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
would be discarded/lost (not ok). They must be returned by a subsequent
`Recv()`.
To resolve this, only remove the head `N` bytes from `m_peek_data`.
Currently, when the FuzzedDataProvider of a FuzzedSock runs out of data,
FuzzedSock::Wait and WaitMany will simulate endless waiting as the
requested events are never simulated as occured.
Fix this by simulating event occurence when ConsumeBool() returns false
(e.g. when the data provider runs out).
Co-authored-by: dergoegge <n.goeggi@gmail.com>
FuzzedSock only supports peeking at one byte at a time, which is not
fuzzer friendly when trying to receive long data.
Fix this by supporting peek data of arbitrary length instead of only one
byte.
In the process of doing so, refactor `ConsumeNetAddr()` to generate the
addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way -
by preparing some random stream and deserializing from it. Similar code
was already found in `RandAddr()`.
There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
Unfortunately, the order of evaluation of function arguments is unspecified. This means it can differ
between compilers/version/optimization levels etc. But when the evaluation order changes, the same
fuzzing input will produce different output, which is bad for coverage/reproducibility.
This PR fixes all these cases where by moving multiple calls to `fuzzed_data_provider` out of the
function arguments.
7df450836969b81e98322c9a09c08b35d1095a25 test: improve sock_tests/move_assignment (Vasil Dimov)
5086a99b84367a45706af7197da1016dd966e6d9 net: remove Sock default constructor, it's not necessary (Vasil Dimov)
7829272f7826511241defd34954e6040ea963f07 net: remove now unnecessary Sock::Get() (Vasil Dimov)
944b21b70ae490a5a746bcc1810a5074d74e9d34 net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov)
aeac68d036e3cff57ce155f1a904d77f98b357d4 net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov)
5ac1a51ee5a57da59f1ff1986b7d9054484d3c80 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing.
Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.
The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it.
ACKs for top commit:
ajtowns:
ACK 7df450836969b81e98322c9a09c08b35d1095a25
jonatack:
ACK 7df450836969b81e98322c9a09c08b35d1095a25
Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
0eeb9b0442fb2f2da33c04704eefe6a84d28e981 [fuzz] Move ConsumeNetAddr to fuzz/util/net.h (dergoegge)
291c8697d4be0f38635b67880107e39d3ec585ad [fuzz] Make ConsumeNetAddr produce valid onion addresses (dergoegge)
c9ba3f836e1646875d2f96f1f466f8a83634a6f7 [netaddress] Make OnionToString public (dergoegge)
Pull request description:
The chance that the fuzzer is able to guess a valid onion address is probably slim, as they are Base32 encoded and include a checksum. Right now, any target using `ConsumeNetAddr` would have a hard time uncovering bugs that require valid onion addresses as input.
This PR makes `ConsumeNetAddr` produce valid onion addresses by using the 32 bytes given by the fuzzer as the pubkey for the onion address and forming a valid address according to the torv3 spec.
ACKs for top commit:
vasild:
ACK 0eeb9b0442fb2f2da33c04704eefe6a84d28e981
brunoerg:
ACK 0eeb9b0442fb2f2da33c04704eefe6a84d28e981
Tree-SHA512: 7c687a4d12f9659559be8f0c3cd4265167d1261d419cfd3d503fd7c7f207cc0db745220f02fb1737e4a5700ea7429311cfc0b42e6c15968ce6a85f8813c7e1d8
