tests: fix -fsanitize=integer complaints #19713
Conversation
|
@Crypt-iQ While you're at it touching (Line 50 is after applying this patch.) Base64 encoded test case: |
8fdf71c to
8209c4d
Compare
|
There is one other complaint in the fuzz directory: Lines 45 to 47 in 3ab2582 Not entirely sure what value to set |
Perhaps you can use |
3c4a4ad to
39a2f5d
Compare
|
Get this complaint on macOS Catalina v10.15.4 with Can't reproduce on my ubuntu digitalocean box with |
|
I can reproduce this on macOS 10.15.5 with clang installed from |
|
@adaminsky what are the options you pass to |
|
@Crypt-iQ I used |
|
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. |
|
@adaminsky If you also run with: Do you also get the following complaint? It hits |
|
These two complaints happen because |
|
The first complaint happens because of the conversion from Lines 35 to 39 in 197450f The second complaint happens because a Lines 70 to 71 in 197450f and bitcoin/src/support/allocators/zeroafterfree.h Lines 45 to 46 in 78dae8c Curious as to your thoughts about these @practicalswift. In the first example, perhaps |
|
@Crypt-iQ Confirming that I can reproduce both complaints and that |
There was a problem hiding this comment.
Concept ACK.
In the first example, perhaps ConsumeRandomLengthString could return std::basic_string. Haven't figured out what to do about the second example.
ConsumeRandomLengthString is part of LLVM's header i don't think it's a good idea to modify it.
Regarding the second complaint, you could directly consume a string without the cast to uint8_t from ConsumeRandomLengthByteVector:
index ae47a2805..fb89dc713 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -38,9 +38,15 @@ NODISCARD inline std::vector<uint8_t> ConsumeRandomLengthByteVector(FuzzedDataPr
return {s.begin(), s.end()};
}
+NODISCARD inline std::vector<char> ConsumeRandomLengthCharVector(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096) noexcept
+{
+ const std::string s = fuzzed_data_provider.ConsumeRandomLengthString(max_length);
+ return {s.begin(), s.end()};
+}
+
NODISCARD inline CDataStream ConsumeDataStream(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096) noexcept
{
- return {ConsumeRandomLengthByteVector(fuzzed_data_provider, max_length), SER_NETWORK, INIT_PROTO_VERSION};
+ return {ConsumeRandomLengthCharVector(fuzzed_data_provider, max_length), SER_NETWORK, INIT_PROTO_VERSION};
}
NODISCARD inline std::vector<std::string> ConsumeRandomLengthStringVector(FuzzedDataProvider& fuzzed_data_provider, const size_t max_vector_size = 16, const size_t max_string_length = 16) noexcept
Regarding the first complaint, ConsumeBytes:
// Returns a std::vector containing |num_bytes| of input data. If fewer than
// |num_bytes| of data remain, returns a shorter std::vector containing all
// of the data that's left. Can be used with any byte sized type, such as
// char, unsigned char, uint8_t, etc.
template <typename T> std::vector<T> ConsumeBytes(size_t num_bytes) {
num_bytes = std::min(num_bytes, remaining_bytes_);
return ConsumeBytes<T>(num_bytes, num_bytes);
}Is almost what you want, as it's just missing the "random length" part. Maybe our ConsumeRandomLengthByteVector could use a combination of this one combined with ConsumeRandomLengthString's randomness logic ?
782cd34 to
e677639
Compare
There was a problem hiding this comment.
Some additional review :)
Since -fsanitize=integer's unsigned-integer-overflow and implicit-integer-sign-change are not UB (but could be indications of bugs) I think it makes sense to be somewhat liberal with tackling these cases by adding suppressions instead of harness changes. Rationales given in each comment.
e677639 to
3e92f88
Compare
…ppressions This is a first step towards getting -fsanitize=integer to not report complaints when fuzzing. Without this, the implicit-integer-sign-change and unsigned-integer-overflow checks are triggered for pretty much every fuzz target. A suppression is added for memory* since macOS builds can trigger implicit-integer-sign-change when using either ConsumeRandomLengthByteVector or ConsumeDeserializable.
This commit also uses AdditionOverflow to prevent overflow.
3e92f88 to
d698ead
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Closed by #21000 |
First steps towards running fuzz tests with
-fsanitize=integerand not having it fail. This PR fixes the complaints in thesrc/test/fuzzdirectory only. There are other complaints that are not handled because I thought the scope of the PR would sprawl.c0f40ba fixes
03826e5 fixes
febe0f8 fixes
d698ead fixes