Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 10 additions & 8 deletions source/common/access_log/access_log_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -262,15 +262,17 @@ MetadataFilter::MetadataFilter(const envoy::config::accesslog::v3::MetadataFilte
: default_match_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(filter_config, match_if_key_not_found, true)),
filter_(filter_config.matcher().filter()) {

auto& matcher_config = filter_config.matcher();
if (filter_config.has_matcher()) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add an explicit test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely -- done

auto& matcher_config = filter_config.matcher();

for (const auto& seg : matcher_config.path()) {
path_.push_back(seg.key());
}
for (const auto& seg : matcher_config.path()) {
path_.push_back(seg.key());
}

// Matches if the value equals the configured 'MetadataMatcher' value.
const auto& val = matcher_config.value();
value_matcher_ = Matchers::ValueMatcher::create(val);
// Matches if the value equals the configured 'MetadataMatcher' value.
const auto& val = matcher_config.value();
value_matcher_ = Matchers::ValueMatcher::create(val);
}

// Matches if the value is present in dynamic metadata
auto present_val = envoy::type::matcher::v3::ValueMatcher();
Expand All @@ -286,7 +288,7 @@ bool MetadataFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::Re
// If the key corresponds to a set value in dynamic metadata, return true if the value matches the
// the configured 'MetadataMatcher' value and false otherwise
if (present_matcher_->match(value)) {
return value_matcher_->match(value);
return value_matcher_ && value_matcher_->match(value);
}

// If the key does not correspond to a set value in dynamic metadata, return true if
Expand Down
26 changes: 26 additions & 0 deletions test/common/access_log/access_log_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,32 @@ name: accesslog
EXPECT_CALL(*file_, write(_)).Times(0);
}

// This is a regression test for fuzz bug https://oss-fuzz.com/testcase-detail/4863844862918656
// where a missing matcher would attempt to create a ValueMatcher and crash in debug mode. Instead,
// the configured metadata filter does not match.
TEST_F(AccessLogImplTest, MetadataFilterNoMatcher) {
const std::string yaml = R"EOF(
name: accesslog
filter:
metadata_filter:
match_if_key_not_found: false
typed_config:
"@type": type.googleapis.com/envoy.config.accesslog.v2.FileAccessLog
path: /dev/null
)EOF";

TestStreamInfo stream_info;
ProtobufWkt::Struct metadata_val;
stream_info.setDynamicMetadata("some.namespace", metadata_val);

const InstanceSharedPtr log =
AccessLogFactory::fromProto(parseAccessLogFromV3Yaml(yaml), context_);

// If no matcher is set, then expect no logs.
EXPECT_CALL(*file_, write(_)).Times(0);
log->log(&request_headers_, &response_headers_, &response_trailers_, stream_info);
}

TEST_F(AccessLogImplTest, MetadataFilterNoKey) {
const std::string default_true_yaml = R"EOF(
name: accesslog
Expand Down
22 changes: 22 additions & 0 deletions test/common/http/conn_manager_impl_corpus/status_163

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
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

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

Large diffs are not rendered by default.