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