fuzz: fixes oss-fuzz: 9895 #4189
Conversation
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
| @@ -143,7 +143,10 @@ RequestInfoHeaderFormatter::RequestInfoHeaderFormatter(absl::string_view field_n | |||
| } | |||
| field_extractor_ = [this, pattern](const Envoy::RequestInfo::RequestInfo& request_info) { | |||
| const auto& formatters = start_time_formatters_.at(pattern); | |||
| ASSERT(formatters.size() == 1); | |||
| if (formatters.size() != 1) { | |||
There was a problem hiding this comment.
I think yes. From the current use-case perspective, this is correct.
There was a problem hiding this comment.
On the second thought, I think we can support multiple patterns.
@anirudhmurali Do you want to take it?
| @@ -0,0 +1,8 @@ | |||
| headers_to_add { | |||
There was a problem hiding this comment.
@anirudhmurali can you add a test case to the unit tests for header_formatter.cc? Best practice is to check in both corpus and a unit test for the new error handling.
| ASSERT(formatters.size() == 1); | ||
| if (formatters.size() != 1) { | ||
| throw EnvoyException( | ||
| fmt::format("multiple start_time values found: {}", formatters.size())); |
There was a problem hiding this comment.
@anirudhmurali, thanks for checking on this.
Questions:
1. Do you think "multiple %START_TIME% patterns..." sounds better?
2. Is the input here: https://github.com/envoyproxy/envoy/pull/4189/files#diff-084669b1d67ce3df5fbda83bef9354bbR3 causing this?
There was a problem hiding this comment.
NVM, I tried to patch the header_formatter to accept this pattern from the corpus, and it seems it makes sense to have multiple patterns inline as the header value.
@anirudhmurali, as I said above, I can work with you to fix this.
(This fuzzer is really good then. This is my mistake, sorry).
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
|
@dio With regards to the Slack discussion and the GitHub comments, I believe this allows definition of multiple |
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
| @@ -617,6 +617,10 @@ match: { prefix: "/new_endpoint" } | |||
| key: "x-request-start" | |||
| value: "%START_TIME(%s.%3f)%" | |||
| append: true | |||
| - header: | |||
| key: "x-request-start" | |||
There was a problem hiding this comment.
Let's rename the key to something else, e.g. "x-request-start-multiple", and add some similar expectations below.
EXPECT_TRUE(headerMap.has("x-request-start-multiple"));
EXPECT_EQ("1522796769.123 2018-04-03T23:06:09.123Z 1522796769", headerMap.get_("x-request-start-multiple"));
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
There was a problem hiding this comment.
LGTM. Thanks, @anirudhmurali!
@htuch sure! Will do that soon after the CI is done the checking. Thank you!
* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>
Title: Fixes oss-fuzz: 9895
Description: oss-fuzz issue (9895): https://oss-fuzz.com/v2/testcase-detail/6195059702628352
The given input testcase had multiple
START_TIMEformatters as value, a valid input must contain only oneSTART_TIME, throwing exception if invalid.Risk Level: Low
Testing: Tested unit tests (bazel test //test/common/router:header_parser_fuzz_test), built and ran fuzzers with oss-fuzz.
Signed-off-by: Anirudh M m.anirudh18@gmail.com