This PR changes the original size_t sep_pos to std::optional<size_t> sep_post_opt to remove the warning when compiling fuzz tests.
0warning: variable 'sep_pos' may be uninitialized when used here [-Wconditional-uninitialized]
Also, it adds --enable-c++17 flag to CI fuzz scripts.
13@@ -14,7 +14,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
14 {
15 // Encoding: [asmap using 1 bit / byte] 0xFF [addr using 1 bit / byte]
16 bool have_sep = false;
17- size_t sep_pos;
18+ size_t sep_pos = 0;
size_t sep_pos = buffer.size(); and before usage do assert(sep_pos < buffer.size()).
--enable-c++17 on the fuzz tests and then simply using std::optional
Oh, I was hoping you keep working on this. All you need to do is to switch the flag in the ci config for the fuzz test. See git grep '\--enable-c++1' ci
Then #import <optional> and adjust the fix.
Oh, I was hoping you keep working on this. All you need to do is to switch the flag in the ci config for the fuzz test. See
git grep '\--enable-c++1' ciThen
#import <optional>and adjust the fix.
No problem, I’ll gladly continue working on it. :)
My impression was, that you are going to introduce new flags and include std::optional which would render this solution obsolete.
14@@ -14,7 +15,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
15 {
16 // Encoding: [asmap using 1 bit / byte] 0xFF [addr using 1 bit / byte]
17 bool have_sep = false;
@MarcoFalke The two fuzz tests on travis seem to be working fine. No warnings related to sep_pos so far.
However, there is another error message, from the fuzzer itself. Not related to this PR, actually, but worth looking into, I think:
0 [#1](/bitcoin-bitcoin/1/) 0x556d615cabf0 in ArgsManager::ParseParameters(int, char const* const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/util/system.cpp:288:21
0SUMMARY: UndefinedBehaviorSanitizer: nullptr-with-nonzero-offset /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.h:527:69 in
1MS: 0 ; base unit: 0000000000000000000000000000000000000000
20x24,0x5f,0x5b,0x5d,
3$_[]
4artifact_prefix='./'; Test unit written to ./crash-d4f298d1f9cf49b6e356887182b01050ec7ac17f
5Base64: JF9bXQ==
A false positive maybe?
14 void test_one_input(const std::vector<uint8_t>& buffer)
15 {
16 // Encoding: [asmap using 1 bit / byte] 0xFF [addr using 1 bit / byte]
17- bool have_sep = false;
18- size_t sep_pos;
19+ std::optional<size_t> sep_pos = std::nullopt;
= std::nullopt;.
I think the UB problem has to do with this line of code in util/system.cpp
0for (int i = 1; i < argc; i++) {
1 std::string key(argv[i]);
I’ll create a draft PR and test it. If it passes, I’ll open a proper PR.
–EDIT: Currently compiling with fuzzers on my macOS. Let’s see if I can catch this error locally.
std::optional instead of the expected Optional? :)
30 }
31 }
32- if (!have_sep) return; // Needs exactly 1 separator
33- if (buffer.size() - sep_pos - 1 > 128) return; // At most 128 bits in IP address
34+ if (!sep_pos) return; // Needs exactly 1 separator
35+ if (buffer.size() - sep_pos.value() - 1 > 128) return; // At most 128 bits in IP address
for a smaller diff you could rename the optional to sep_pos_opt and insert before this line
0const size_t sep_pos{sep_pos_opt.value()};
ACK 848ab4918b88f9095c49ed7aaa9c12558b1fdda3
Seems reasonable to switch to C++17 for the fuzzers. Locally I am already running them on C++17, and the project is going to switch in about 4-5 months as well. So I don’t see the need to additionally burden the fuzz tests with the heavy boost headers temporarily and then make an effort to pull them out again.
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
30 return;
31 }
32 }
33- if (!have_sep) return; // Needs exactly 1 separator
34+ const size_t sep_pos{sep_pos_opt.value()};
35+ if (!sep_pos) return; // Needs exactly 1 separator
The two lines are swapped, no?
0 if (!sep_pos_opt) return; // Needs exactly 1 separator
Yeah, ~these errors happen when you have no working compiler on your machine~. ~Can’t compile fuzzing variants anymore~ (Catalina is not an OS but Malware). It’s time for Linux VM.
Sorry!
–EDIT: it works now, somehow.
Labels
Tests