true is the normal case, while false indicates the input was uninteresting and should not under any circumstances be added to the corpus. This is intended for fuzz targets that have some early bail-out criteria, so that the fuzzer doesn’t continue to iterate on them.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Can you add some context to explain how this interacts with fuzzing engines? Will this make it harder for engines to start from an empty input set? Often, to find a sufficiently long input to pass basic deserialization, fuzz engines will have to be guided, for example -use_value_profile=1 for libfuzzer, and discarding the inputs on the way would mean they will never succeed passing basic deserialization?
Moreover, it could help to state a goal. Is it to keep the qa-assets repo small?
32+/** Fuzz target without initialization function that always succeeds. */
33 #define FUZZ_TARGET(name) \
34 FUZZ_TARGET_INIT(name, FuzzFrameworkEmptyInitFun)
35
36+/** Fuzz target without initialization function that returns bool (false = uninteresting test). */
37+#define FUZZ_PARTIAL_TARGET(name) \
LLVMFuzzerTestOneInput that don’t support the return value -1 at all, we should make sure it also isn’t returned, even if FUZZ_PARTIAL_TARGET is used).
150@@ -151,8 +151,8 @@ void signal_handler(int signal)
151 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
152 {
153 static const auto& test_one_input = *Assert(g_test_one_input);
154- test_one_input({data, size});
155- return 0;
156+ /* Returning -1 means the input was not useful. */
157+ return int{test_one_input({data, size})} - 1;
Just noting that this was only recently added to libFuzzer: https://reviews.llvm.org/D128749?id=441094
I think that is fine, running with older versions of libFuzzer makes little sense anyway.
Suggestion, if you want to go down the route to make this a runtime option:
0static const reject_unwated_inputs{std::getenv("REJECT_UNWANTED_FUZZ_INPUTS")};
(or similar)
@MarcoFalke Perhaps, but I don’t worry too much if it’s used conservatively. The “having to go through uninteresting cases to get to interesting ones” is a concern with or without this functionality, because after all, the uninteresting cases are already unlikely to trigger much (useful) coverage, and the coverage that they do trigger is likely unrelated to what is interesting. The actual solution libfuzzer has for this concern is attempting multiple (up to 5, IIRC) mutations in one step.
Of course, (over)use of this feature may make things worse, but that’s up to the individual tests.
Maybe it’s worth experimenting a bit with to so how much impact it has; e.g. introduce old/known bugs into the code, start from an empty corpus, and measure on average how long in time it takes to find the bug, with and without this. The miniscript fuzzers (where I’ve added return false;s relatively liberally in this PR) could be a good guinea pig.
Maybe it’s worth experimenting a bit with to so how much impact it has;
Yes, I’m planning to play with that, I’d be really interested in whether there is a significant speedup.
I feel like ideally, this would be something a good fuzzing engine should be able to handle to some extent without user guidance - uninteresting cases should create fewer additional seeds added to the corpus, which should result in them being picked by the engine for mutation less (and there might be more sophisticated algorithms that would further reduce the time spent on seeds that have failed to create interesting mutations before). That wouldn’t drive the time spent on these uninteresting inputs down to zero like the approach here though.
28@@ -29,22 +29,22 @@ static const std::vector<bool> IPV4_PREFIX_ASMAP = {
29 true, true, false, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true // Match 0xFF
30 };
31
32-FUZZ_TARGET(asmap)
33+FUZZ_PARTIAL_TARGET(asmap)
34 {
35 // Encoding: [7 bits: asmap size] [1 bit: ipv6?] [3-130 bytes: asmap] [4 or 16 bytes: addr]
36- if (buffer.size() < 1 + 3 + 4) return;
37+ if (buffer.size() < 1 + 3 + 4) return false;
enum class to avoid confusion due to under-documentation and accidental typo bugs?
bool to an enum class FuzzResult which has values MAYBE_INTERESTING and UNINTERESTING, making it hopefully clearer what the return values correspond to.
46 for (int j = 0; j < 8; ++j) {
47 asmap.push_back((buffer[1 + i] >> j) & 1);
48 }
49 }
50- if (!SanityCheckASMap(asmap, 128)) return;
51+ if (!SanityCheckASMap(asmap, 128)) return FuzzResult::MAYBE_INTERESTING;
0 if (!SanityCheckASMap(asmap, 128)) return FuzzResult::UNINTERESTING;
SanityCheckASMap into the qa-assets directory.
I did a quick test. I suppose that with this new approach, miniscript_script corpus will contain only valid miniscripts, this sounds good. So, I first ran: FUZZ=miniscript_script src/test/fuzz/fuzz new_corpus -runs=1000000. And then I “fuzzed” decodescript RPC using the following script I created:
0#!/usr/bin/env python3
1
2import sys
3sys.path.insert(0, "/path/to/test/functional")
4from test_framework.test_shell import TestShell
5
6import binascii
7import os
8
9def miniscript():
10 dirc = '/path/to/corpus/'
11 test = TestShell().setup(num_nodes=1, setup_clean_chain=True)
12 node = test.nodes[0]
13 for file in os.listdir(dirc):
14 with open(os.path.join(dirc, file), 'rb') as f:
15 byte_data = f.read()
16 hex_string = binascii.hexlify(byte_data).decode('utf-8')
17 res = node.decodescript(hex_string)
18 print(res)
19
20 test.shutdown()
21
22if __name__ == '__main__':
23 miniscript()
It worked as expected.
For specific cases I think this approach (indicating usefulness) may be useful, for other ones it may be “dangerous”. We can do a simple mutation in an interesting result and it may become an uninteresting one, then we can do another mutation and it becomes an interesting one - different from the first case.
I did other test to evaluate this approach in other scenario.
In addrman harness, we have:
0const AddrMan& const_addr_man{addr_man};
1(void)const_addr_man.GetAddr(
2 /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
3 /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
4 /*network=*/std::nullopt);
5(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool());
6(void)const_addr_man.Size();
I believe that calling GetAddr, Select and other functions may not be so useful if the addrman is empty. So, using “partial” fuzzers, we could do:
0if (addr_man.Size() == 0) return FuzzResult::UNINTERESTING;
1const AddrMan& const_addr_man{addr_man};
2(void)const_addr_man.GetAddr(
3 /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
4 /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
5 /*network=*/std::nullopt);
6(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool());
7(void)const_addr_man.Size();
8CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
9data_stream << const_addr_man;
10if (addr_man.Size() == 0) return FuzzResult::MAYBE_INTERESTING;
I ran it and the result was:
0-➜ bitcoin-core-dev git:(27552-sipa) ✗ FUZZ=addrman src/test/fuzz/fuzz -runs=100000 -print_final_stats=1
1INFO: Running with entropic power schedule (0xFF, 100).
2INFO: Seed: 3233727686
3INFO: Loaded 1 modules (1141182 inline 8-bit counters): 1141182 [0x107f53780, 0x10806a13e),
4INFO: Loaded 1 PC tables (1141182 PCs): 1141182 [0x10806a140,0x1091d3d20),
5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
6INFO: A corpus is not provided, starting from an empty corpus
7[#2](/bitcoin-bitcoin/2/) INITED exec/s: 0 rss: 105Mb
8WARNING: no interesting inputs were found so far. Is the code instrumented for coverage?
9This may also happen if the target rejected all inputs we tried so far
10[#100000](/bitcoin-bitcoin/100000/) DONE corp: 1/1b lim: 994 exec/s: 3225 rss: 422Mb
11Done 100000 runs in 31 second(s)
12stat::number_of_executed_units: 100000
13stat::average_exec_per_sec: 3225
14stat::new_units_added: 0
15stat::slowest_unit_time_sec: 0
16stat::peak_rss_mb: 422
However, if instead of doing that, we just do a “return” if addrman is empty, would it be more effective? E.g.
0if (addr_man.Size() == 0) return;
1else assert(false);
2const AddrMan& const_addr_man{addr_man};
3(void)const_addr_man.GetAddr(
4 /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
5 /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
6 /*network=*/std::nullopt);
7(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool());
8(void)const_addr_man.Size();
9CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
10data_stream << const_addr_man;
note: I added an assert to crash as soon as addrman is not empty anymore.
I ran same command that I did previously and the result was:
0SUMMARY: libFuzzer: deadly signal
1MS: 5 ChangeBit-ChangeBinInt-ChangeASCIIInt-CrossOver-CopyPart-; base unit: 7cf855d3c582971ea888061d02610fe375f68776
20x5c,0xff,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x26,0xff,0x52,0x44,0x36,0xff,0xff,0x0,0x0,0x0,0x49,0x5c,0xff,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x7a,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x3f,0x41,0x3f,0x54,0x7e,0x54,0x8f,0x41,
3\\\377zzzzzzzzz\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000&\377RD6\377\377\000\000\000I\\\377zzzzzzzzz\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000?A?T~T\217A
4artifact_prefix='./'; Test unit written to ./crash-cf486b4859d3e46c3591f9a71e2f83dc384d3987
5Base64: XP96enp6enp6enoAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAJv9SRDb//wAAAElc/3p6enp6enp6egAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP0E/VH5Uj0E=
6stat::number_of_executed_units: 38666
7stat::average_exec_per_sec: 1933
8stat::new_units_added: 261
9stat::slowest_unit_time_sec: 0
10stat::peak_rss_mb: 351
It executed 38666 units and crashed because addrman is not empty anymore. It seemed to be more effective.
44+/** Fuzz target without initialization function that always succeeds. */
45 #define FUZZ_TARGET(name) \
46 FUZZ_TARGET_INIT(name, FuzzFrameworkEmptyInitFun)
47
48+/** Fuzz target without initialization function that returns FuzzResult. */
49+#define FUZZ_PARTIAL_TARGET(name) \
nit: (This is my fault)
Not really a fan adding more macros, where each new option will cause doubling all existing macros. Currently there are 3, in this pull there are 6, and with the next option we’ll have 12 to 16 macros?
At least for the existing options, which only need to be known at runtime, an options struct can be used.
See #28065 . Feel free to ignore/NACK.
Edit: To clarify having FUZZ_TARGET and FUZZ_PARTIAL_TARGET is probably fine. My comment was about the other macros in other lines.
1110- if (!script) return;
1111+ if (!script) return FuzzResult::UNINTERESTING;
1112
1113 const auto ms = miniscript::FromScript(*script, SCRIPT_PARSER_CONTEXT);
1114- if (!ms) return;
1115+ if (!ms) return FuzzResult::UNINTERESTING;
miniscript::FromScript fails? This seems undesirable, because then someone can change the code to add undefined behavior or a crash in code paths that return an error.
miniscript_script and miniscript_string could be left more generic by not discarding any coverage while miniscript_smart and miniscript_stable would.
1089 {
1090 FuzzedDataProvider provider(buffer.data(), buffer.size());
1091 auto str = provider.ConsumeRemainingBytesAsString();
1092 auto parsed = miniscript::FromString(str, PARSER_CTX);
1093- if (!parsed) return;
1094+ if (!parsed) return FuzzResult::UNINTERESTING;
28+
29+ /** This value can be returned by fuzz tests to indicate the input was uninteresting.
30+ *
31+ * libfuzzer can make use of this and will not insert the input in its corpus, even when it
32+ * appears to increase coverage. */
33+ UNINTERESTING
0 UNINTERESTING,
Style nit: Missing comma to avoid having to touch this line again if a new value is added (unlikely).
165@@ -166,8 +166,9 @@ void signal_handler(int signal)
166 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
167 {
168 static const auto& test_one_input = *Assert(g_test_one_input);
169- test_one_input({data, size});
170- return 0;
171+ auto result = test_one_input({data, size});
172+ /* Returning -1 means the input was not useful. */
173+ return (result != FuzzResult::UNINTERESTING) - 1;
🐙 This pull request conflicts with the target branch and needs rebase.
Rebase