Skip to content

[test] fix fuzz tests that might crash on duplicate settings params #10779

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 3 commits into from
Apr 20, 2020
Merged

[test] fix fuzz tests that might crash on duplicate settings params #10779

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75a2537
fix request
asraa Apr 14, 2020
3f1ba02
fix, because we don't want to block duplicate settings
asraa Apr 14, 2020
280f121
use unordered map
asraa Apr 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion test/common/http/http2/codec_impl_test_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,16 @@ class TestCodecSettingsProvider {
void onSettingsFrame(const nghttp2_settings& settings_frame) {
for (uint32_t i = 0; i < settings_frame.niv; ++i) {
auto result = settings_.insert(settings_frame.iv[i]);
ASSERT(result.second);
// It is possible to have duplicate settings parameters, each new parameter replaces any
// existing value.
// https://tools.ietf.org/html/rfc7540#section-6.5
if (!result.second) {
ENVOY_LOG_MISC(debug, "Duplicated settings parameter {} with value {}",
settings_frame.iv[i].settings_id, settings_frame.iv[i].value);
settings_.erase(result.first);

@AndresGuedez AndresGuedez Apr 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems preferable to use a map instead now that uniqueness checks are not required.

@antoniovicente antoniovicente Apr 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on what the intent of this helper was. If it was to detect erroneous accidental double settings in tests, then the original was best.

It's a bit strange that we do a set of nghttp2_settings_entry instead of a map<settings_id, value> and fail when setting_id is duplicated. I can see us doing the nghttp2_settings_entry if we're trying to avoid errorneously sending the exact same setting_id, value pair multiple times.

@AndresGuedez AndresGuedez Apr 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent was to simply provide a helper to fetch SETTINGS parameters received from a peer. The use of the set was based on my misunderstanding of the RFC; the goal was to be very explicit about the uniqueness requirement through the type used.

@AndresGuedez AndresGuedez Apr 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on Antonio's point about accidentally sending duplicate pairs, we should add a test that validates that condition does not happen, but this utility should be generic enough to support both the existing unit tests, fuzz tests and to-be-written duplicate parameter tests.

@asraa asraa Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to an unordered-map. this isn't relevant to sendSettings, which fwiw parses configuration settings for your codecs. (And we can impose restrictions like do not duplicate settings in your config). I can write a test that sends a settingsframe containing duplicate settings in a frame (this would have always worked against a real envoy, since this is just test utility)

// Guaranteed success here.
ASSERT(settings_.insert(settings_frame.iv[i]).second);

@htuch htuch Apr 20, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body of ASSERTs can't be side effecting, since they disappear in opt build (and we do run tests on opt builds)..

@asraa asraa Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, in hindsight that was pretty silly. Thank you!

}
}
}

Expand Down
Binary file added ...ader_corpus/clusterfuzz-testcase-minimized-request_header_fuzz_test-4795710559223808.fuzz
View file
Open in desktop
Binary file not shown.