Skip to content
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

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

Merged
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 all 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
23 changes: 16 additions & 7 deletions 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 @@ -13,27 +13,36 @@ class TestCodecSettingsProvider {
public:
// Returns the value of the SETTINGS parameter keyed by |identifier| sent by the remote endpoint.
absl::optional<uint32_t> getRemoteSettingsParameterValue(int32_t identifier) const {
const auto it = settings_.find({identifier, 0});
const auto it = settings_.find(identifier);
if (it == settings_.end()) {
return absl::nullopt;
}
return it->value;
return it->second;
}

protected:
// Stores SETTINGS parameters contained in |settings_frame| to make them available via
// getRemoteSettingsParameterValue().
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);
auto result = settings_.insert(
std::make_pair(settings_frame.iv[i].settings_id, settings_frame.iv[i].value));
// 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);
Copy link
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.

Copy link
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.

Copy link
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.

Copy link
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.

Copy link
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.
settings_.insert(
std::make_pair(settings_frame.iv[i].settings_id, settings_frame.iv[i].value));
}
}
}

private:
std::unordered_set<nghttp2_settings_entry, ::Envoy::Http2::Utility::SettingsEntryHash,
::Envoy::Http2::Utility::SettingsEntryEquals>
settings_;
std::unordered_map<int32_t, uint32_t> settings_;
};

class TestServerConnectionImpl : public ServerConnectionImpl, public TestCodecSettingsProvider {
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.