Skip to content

[fuzz] fix bad inputs and config bugs #12504

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
ggreenway merged 5 commits into from
Aug 10, 2020
Merged

[fuzz] fix bad inputs and config bugs #12504

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82e50d9
fix bad inputs and config bugs
asraa Aug 5, 2020
d41b2bf
fix
asraa Aug 5, 2020
3d4d7c6
fix matcher in code
asraa Aug 7, 2020
fbadb05
add unit test
asraa Aug 7, 2020
d47389c
Merge remote-tracking branch 'upstream/master' into hcm-1xx-bug
asraa Aug 10, 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
2 changes: 1 addition & 1 deletion api/envoy/config/accesslog/v3/accesslog.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ message MetadataFilter {
// Matcher to check metadata for specified value. For example, to match on the
// access_log_hint metadata, set the filter to "envoy.common" and the path to
// "access_log_hint", and the value to "true".
type.matcher.v3.MetadataMatcher matcher = 1;
type.matcher.v3.MetadataMatcher matcher = 1 [(validate.rules).message = {required: true}];

@lizan lizan Aug 7, 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.

https://github.com/envoyproxy/envoy/blob/master/api/API_VERSIONING.md#backwards-compatibility

Increasing the strictness of protoc-gen-validate annotations. Exceptions may be granted for scenarios in which these stricter conditions model behavior already implied structurally or by documentation.

Assuming this change falls into this exception, because MetadataFilter can be null, @asraa ?

@asraa asraa Aug 7, 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.

It might be able to be solved in code too. MetadataFilter only needs an actual MetadataMatcher matcher so that it can have a value, otherwise this line crashes in code

envoy/source/common/access_log/access_log_impl.cc

Line 273 in d7c7e9a

value_matcher_ = Matchers::ValueMatcher::create(val);

because of

envoy/source/common/common/matchers.cc

Line 34 in b6d1fec

NOT_REACHED_GCOVR_EXCL_LINE;

Thinking about this more, it is possible to solve in code by only setting value_matcher_ if there's a matcher, and evaluating the match by preconditioning that there's a value_matcher_.
I think code may be better (at least for now)

@asraa asraa Aug 7, 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.

Opted for the code change.


// Default result if the key does not exist in dynamic metadata: if unset or
// true, then log; if false, then don't log.
Expand Down
2 changes: 1 addition & 1 deletion api/envoy/config/accesslog/v4alpha/accesslog.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion generated_api_shadow/envoy/config/accesslog/v3/accesslog.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion generated_api_shadow/envoy/config/accesslog/v4alpha/accesslog.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions test/common/http/conn_manager_impl_corpus/status_163
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion test/common/http/conn_manager_impl_fuzz_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,11 +477,17 @@ class FuzzStream {
Fuzz::fromHeaders<TestResponseHeaderMapImpl>(response_action.headers()));
// The client codec will ensure we always have a valid :status.
// Similarly, local replies should always contain this.
uint64_t status;
try {
Utility::getResponseStatus(*headers);
status = Utility::getResponseStatus(*headers);
} catch (const CodecClientException&) {
headers->setReferenceKey(Headers::get().Status, "200");
}
// The only 1xx header that may be provided to encodeHeaders() is a 101 upgrade,
// guaranteed by the codec parsers. See include/envoy/http/filter.h.
if (CodeUtility::is1xx(status) && status != enumToInt(Http::Code::SwitchingProtocols)) {
headers->setReferenceKey(Headers::get().Status, "200");
}
decoder_filter_->callbacks_->encodeHeaders(std::move(headers), end_stream);
state = end_stream ? StreamState::Closed : StreamState::PendingDataOrTrailers;
}
Expand Down
7 changes: 7 additions & 0 deletions test/extensions/filters/http/common/fuzz/filter_corpus/metadata_not_reached
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions test/integration/h1_corpus/stream_info_destructor
View file
Open in desktop

Large diffs are not rendered by default.