From adb6e5a98f6ea6dd6c16f0040a28129b3a900fe2 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 8 May 2023 19:16:04 +0000 Subject: [PATCH 1/7] Adding ext_proc filter config and response timeout Duration PGVs to avoid ext_proc filter fuzzer crash due to duration config out-of-bounds. Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/v3/ext_proc.proto | 10 ++++-- .../ext_proc/v3/external_processor.proto | 5 ++- ...h-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 diff --git a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto index b916f874f30b..2abb7bfd2df6 100644 --- a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto +++ b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto @@ -145,7 +145,10 @@ message ExternalProcessor { // to the processing mode) if the timer expires before a matching response // is received. There is no timeout when the filter is running in asynchronous // mode. Default is 200 milliseconds. - google.protobuf.Duration message_timeout = 7; + google.protobuf.Duration message_timeout = 7 [(validate.rules).duration = { + lte {seconds: 3600} + gte {} + }]; // Optional additional prefix to use when emitting statistics. This allows to distinguish // emitted statistics between configured *ext_proc* filters in an HTTP filter chain. @@ -167,7 +170,10 @@ message ExternalProcessor { // Specify the upper bound of // :ref:`override_message_timeout ` // If not specified, by default it is 0, which will effectively disable the ``override_message_timeout`` API. - google.protobuf.Duration max_message_timeout = 10; + google.protobuf.Duration max_message_timeout = 10 [(validate.rules).duration = { + lte {seconds: 3600} + gte {} + }]; // Prevents clearing the route-cache when the // :ref:`clear_route_cache ` diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index 9d09828e1c50..bab047aed737 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -185,7 +185,10 @@ message ProcessingResponse { // :ref:`max_message_timeout ` // Such message can be sent at most once in a particular Envoy ext_proc filter processing state. // To enable this API, one has to set ``max_message_timeout`` to a number >= 1ms. - google.protobuf.Duration override_message_timeout = 10; + google.protobuf.Duration override_message_timeout = 10 [(validate.rules).duration = { + lte {seconds: 3600} + gte {nanos: 1000000} + }]; } // The following are messages that are sent to the server. diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 new file mode 100644 index 000000000000..ec0f8a51e546 --- /dev/null +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 @@ -0,0 +1,36 @@ +config { + grpc_service { + envoy_grpc { + cluster_name: "#" + retry_policy { + retry_back_off { + base_interval { + seconds: 137438953472 + } + } + } + } + timeout { + seconds: 137438953472 + nanos: 655360 + } + } + request_attributes: "#" + message_timeout { + seconds: 137438953472 + } + max_message_timeout { + seconds: 137438953472 + } + disable_clear_route_cache: true +} +request { +} +response { + request_headers { + } + override_message_timeout { + seconds: 137438953472 + nanos: 655360 + } +} From 2554501ffb4c0677187d8c3695c1be18aaa357a2 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 9 May 2023 04:25:30 +0000 Subject: [PATCH 2/7] Adding exception handling for response with large override_message_timeout Signed-off-by: Yanjun Xiang --- .../service/ext_proc/v3/external_processor.proto | 5 +---- source/extensions/filters/http/ext_proc/ext_proc.cc | 11 ++++++++++- .../http/ext_proc/ext_proc_integration_test.cc | 13 +++++++++++-- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index bab047aed737..9d09828e1c50 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -185,10 +185,7 @@ message ProcessingResponse { // :ref:`max_message_timeout ` // Such message can be sent at most once in a particular Envoy ext_proc filter processing state. // To enable this API, one has to set ``max_message_timeout`` to a number >= 1ms. - google.protobuf.Duration override_message_timeout = 10 [(validate.rules).duration = { - lte {seconds: 3600} - gte {nanos: 1000000} - }]; + google.protobuf.Duration override_message_timeout = 10; } // The following are messages that are sent to the server. diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index f849c0fb4794..92f9606afb00 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -559,7 +559,16 @@ void Filter::onReceiveMessage(std::unique_ptr&& r) { // Check whether the server is asking to extend the timer. if (response->has_override_message_timeout()) { - onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout())); + // The override_message_timeout in response may be too big for duration, which leads + // to exception been thrown during durationToMilliseconds() check. + // Needs to properly handle this case. + try { + onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout())); + } catch (const DurationUtil::OutOfRangeException& e) { + ENVOY_LOG(warn, "override_message_timeout value out-of-range: {}. Ignoring the message.", + e.what()); + stats_.override_message_timeout_ignored_.inc(); + } return; } diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 55fb05d10d0f..1adb3a327463 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -315,7 +315,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, // ext_proc server sends back a response to tell Envoy to stop the // original timer and start a new timer. - void serverSendNewTimeout(const uint32_t timeout_ms) { + void serverSendNewTimeout(const uint64_t timeout_ms) { ProcessingResponse response; if (timeout_ms < 1000) { response.mutable_override_message_timeout()->set_nanos(timeout_ms * 1000000); @@ -327,7 +327,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, // The new timeout message is ignored by Envoy due to different reasons, like // new_timeout setting is out-of-range, or max_message_timeout is not configured. - void newTimeoutWrongConfigTest(const uint32_t timeout_ms) { + void newTimeoutWrongConfigTest(const uint64_t timeout_ms) { // Set envoy filter timeout to be 200ms. proto_config_.mutable_message_timeout()->set_nanos(200000000); // Config max_message_timeout proto to enable the new timeout API. @@ -1960,4 +1960,13 @@ TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutNegativeTestTimeoutNotAcc newTimeoutWrongConfigTest(500); } +// Send the new timeout to be an extremely large number, which will trigger exception being thrown. +// Verify the code appropriately handled it. +TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutOutOfBounds) { + // Config max_message_timeout proto to 100ms to enable the new timeout API. + max_message_timeout_ms_ = 100; + const uint64_t override_message_timeout = 1000000000000000; + newTimeoutWrongConfigTest(override_message_timeout); +} + } // namespace Envoy From f79685371b9cadc2fe8eb56b912a27163609e4d4 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 10 May 2023 01:09:57 +0000 Subject: [PATCH 3/7] addressing comments Signed-off-by: Yanjun Xiang --- source/common/protobuf/utility.cc | 41 +++++++++++++++---- source/common/protobuf/utility.h | 9 ++++ .../filters/http/ext_proc/ext_proc.cc | 24 +++++------ .../filters/http/ext_proc/ext_proc.h | 2 +- .../ext_proc/ext_proc_integration_test.cc | 2 +- 5 files changed, 55 insertions(+), 23 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 7cdc7d5042a8..963587c39d9e 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -645,34 +645,57 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector& v namespace { -void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { +void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value, + bool throw_exception, bool& error) { + error = false; if (duration.seconds() < 0 || duration.nanos() < 0) { - throw DurationUtil::OutOfRangeException( - fmt::format("Expected positive duration: {}", duration.DebugString())); + if (throw_exception) { + throw DurationUtil::OutOfRangeException( + fmt::format("Expected positive duration: {}", duration.DebugString())); + } else { + error = true; + return; + } } if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { - throw DurationUtil::OutOfRangeException( - fmt::format("Duration out-of-range: {}", duration.DebugString())); + if (throw_exception) { + throw DurationUtil::OutOfRangeException( + fmt::format("Duration out-of-range: {}", duration.DebugString())); + } else { + error = true; + } } } void validateDuration(const ProtobufWkt::Duration& duration) { - validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds); + bool error = false; + validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds, true, error); } -void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) { +void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration, bool throw_exception, + bool& error) { // Apply stricter max boundary to the `seconds` value to avoid overflow. // Note that protobuf internally converts to nanoseconds. // The kMaxInt64Nanoseconds = 9223372036, which is about 300 years. constexpr int64_t kMaxInt64Nanoseconds = std::numeric_limits::max() / (1000 * 1000 * 1000); - validateDuration(duration, kMaxInt64Nanoseconds); + validateDuration(duration, kMaxInt64Nanoseconds, throw_exception, error); } } // namespace uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& duration) { - validateDurationAsMilliseconds(duration); + bool error = false; + validateDurationAsMilliseconds(duration, true, error); + return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); +} + +uint64_t DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, + bool& error) { + validateDurationAsMilliseconds(duration, false, error); + if (error) { + return 0; + } return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 7f5847e310a7..080abc2ecd3b 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -683,6 +683,15 @@ class DurationUtil { */ static uint64_t durationToMilliseconds(const ProtobufWkt::Duration& duration); + /** + * Same as DurationUtil::durationToMilliseconds but does not throw an exception. + * Instead, it returns error if duration is out-of-range. + * @param duration protobuf. + * @param error bool, set into true in case of duration out-of-range; otherwise false. + * @return duration in milliseconds. + */ + static uint64_t durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, bool& error); + /** * Same as Protobuf::util::TimeUtil::DurationToSeconds but with extra validation logic. * Specifically, we ensure that the duration is positive. diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 92f9606afb00..a538bcc67054 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -527,12 +527,21 @@ void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers stats_.stream_msgs_sent_.inc(); } -void Filter::onNewTimeout(const uint32_t message_timeout_ms) { +void Filter::onNewTimeout(const ProtobufWkt::Duration& override_message_timeout) { + bool error = false; + uint32_t message_timeout_ms = + DurationUtil::durationToMillisecondsNoThrow(override_message_timeout, error); + if (error) { + ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of duration range. " + "Ignoring the message."); + stats_.override_message_timeout_ignored_.inc(); + return; + } // The new timeout has to be >=1ms and <= max_message_timeout configured in filter. const uint32_t min_timeout_ms = 1; const uint32_t max_timeout_ms = config_->maxMessageTimeout(); if (message_timeout_ms < min_timeout_ms || message_timeout_ms > max_timeout_ms) { - ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of range. " + ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of config range. " "Ignoring the message."); stats_.override_message_timeout_ignored_.inc(); return; @@ -559,16 +568,7 @@ void Filter::onReceiveMessage(std::unique_ptr&& r) { // Check whether the server is asking to extend the timer. if (response->has_override_message_timeout()) { - // The override_message_timeout in response may be too big for duration, which leads - // to exception been thrown during durationToMilliseconds() check. - // Needs to properly handle this case. - try { - onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout())); - } catch (const DurationUtil::OutOfRangeException& e) { - ENVOY_LOG(warn, "override_message_timeout value out-of-range: {}. Ignoring the message.", - e.what()); - stats_.override_message_timeout_ignored_.inc(); - } + onNewTimeout(response->override_message_timeout()); return; } diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 0bc052997e88..2824cb4d1944 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -191,7 +191,7 @@ class Filter : public Logger::Loggable, void onGrpcClose() override; void onMessageTimeout(); - void onNewTimeout(const uint32_t message_timeout_ms); + void onNewTimeout(const ProtobufWkt::Duration& override_message_timeout); void sendBufferedData(ProcessorState& state, ProcessorState::CallbackState new_state, bool end_stream) { diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 1adb3a327463..1d4565a56470 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -1960,7 +1960,7 @@ TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutNegativeTestTimeoutNotAcc newTimeoutWrongConfigTest(500); } -// Send the new timeout to be an extremely large number, which will trigger exception being thrown. +// Send the new timeout to be an extremely large number, which is out-of-range of duration. // Verify the code appropriately handled it. TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutOutOfBounds) { // Config max_message_timeout proto to 100ms to enable the new timeout API. From b3187e171782b6d0ee129e74a303caf264730032 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 10 May 2023 17:23:20 +0000 Subject: [PATCH 4/7] adding unit test for the newly added NoThrow utility function Signed-off-by: Yanjun Xiang --- test/common/protobuf/utility_test.cc | 57 ++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 030ff3f0ba0e..308af7a6bac8 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1705,6 +1705,63 @@ TEST(DurationUtilTest, OutOfRange) { } } +TEST(DurationUtilTest, NoThrow) { + { + // In range test + ProtobufWkt::Duration duration; + duration.set_seconds(5); + duration.set_nanos(10000000); + bool error = false; + auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); + EXPECT_FALSE(error); + EXPECT_TRUE(ms == 5010); + } + + // Below are out-of-range tests + { + ProtobufWkt::Duration duration; + duration.set_seconds(-1); + bool error = false; + auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); + EXPECT_TRUE(error); + EXPECT_TRUE(ms == 0); + } + { + ProtobufWkt::Duration duration; + duration.set_nanos(-1); + bool error = false; + auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); + EXPECT_TRUE(error); + EXPECT_TRUE(ms == 0); + } + { + ProtobufWkt::Duration duration; + duration.set_nanos(1000000000); + bool error = false; + auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); + EXPECT_TRUE(error); + EXPECT_TRUE(ms == 0); + } + { + ProtobufWkt::Duration duration; + duration.set_seconds(Protobuf::util::TimeUtil::kDurationMaxSeconds + 1); + bool error = false; + auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); + EXPECT_TRUE(error); + EXPECT_TRUE(ms == 0); + } + { + ProtobufWkt::Duration duration; + constexpr int64_t kMaxInt64Nanoseconds = + std::numeric_limits::max() / (1000 * 1000 * 1000); + duration.set_seconds(kMaxInt64Nanoseconds + 1); + bool error = false; + auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); + EXPECT_TRUE(error); + EXPECT_TRUE(ms == 0); + } +} + // Verify WIP accounting of the file based annotations. This test uses the strict validator to test // that code path. TEST_F(ProtobufUtilityTest, MessageInWipFile) { From 217b1851fd0627e4d473d8dd20272e8eb6ecbd6e Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 10 May 2023 18:05:58 +0000 Subject: [PATCH 5/7] addressing comments Signed-off-by: Yanjun Xiang --- source/common/protobuf/utility.cc | 54 +++++++++++++++++-------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 963587c39d9e..8fa5db69ae7e 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -645,55 +645,59 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector& v namespace { -void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value, - bool throw_exception, bool& error) { - error = false; +void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { if (duration.seconds() < 0 || duration.nanos() < 0) { - if (throw_exception) { - throw DurationUtil::OutOfRangeException( - fmt::format("Expected positive duration: {}", duration.DebugString())); - } else { - error = true; - return; - } + throw DurationUtil::OutOfRangeException( + fmt::format("Expected positive duration: {}", duration.DebugString())); } if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { - if (throw_exception) { - throw DurationUtil::OutOfRangeException( - fmt::format("Duration out-of-range: {}", duration.DebugString())); - } else { - error = true; - } + throw DurationUtil::OutOfRangeException( + fmt::format("Duration out-of-range: {}", duration.DebugString())); + } +} + +bool validateDurationErrorNoThrow(const ProtobufWkt::Duration& duration, + int64_t max_seconds_value) { + if (duration.seconds() < 0 || duration.nanos() < 0 || duration.nanos() > 999999999 || + duration.seconds() > max_seconds_value) { + ENVOY_LOG_MISC(warn, "Duration out-of-range: {}", duration.DebugString()); + return true; } + return false; } void validateDuration(const ProtobufWkt::Duration& duration) { - bool error = false; - validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds, true, error); + validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds); +} + +void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) { + // Apply stricter max boundary to the `seconds` value to avoid overflow. + // Note that protobuf internally converts to nanoseconds. + // The kMaxInt64Nanoseconds = 9223372036, which is about 300 years. + constexpr int64_t kMaxInt64Nanoseconds = + std::numeric_limits::max() / (1000 * 1000 * 1000); + validateDuration(duration, kMaxInt64Nanoseconds); } -void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration, bool throw_exception, - bool& error) { +bool validateDurationAsMillisecondsErrorNoThrow(const ProtobufWkt::Duration& duration) { // Apply stricter max boundary to the `seconds` value to avoid overflow. // Note that protobuf internally converts to nanoseconds. // The kMaxInt64Nanoseconds = 9223372036, which is about 300 years. constexpr int64_t kMaxInt64Nanoseconds = std::numeric_limits::max() / (1000 * 1000 * 1000); - validateDuration(duration, kMaxInt64Nanoseconds, throw_exception, error); + return validateDurationErrorNoThrow(duration, kMaxInt64Nanoseconds); } } // namespace uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& duration) { - bool error = false; - validateDurationAsMilliseconds(duration, true, error); + validateDurationAsMilliseconds(duration); return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); } uint64_t DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, bool& error) { - validateDurationAsMilliseconds(duration, false, error); - if (error) { + if ((error = validateDurationAsMillisecondsErrorNoThrow(duration))) { return 0; } return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); From 9ccba29707756825e183a9c4917caeb15b2253aa Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 10 May 2023 20:56:35 +0000 Subject: [PATCH 6/7] addressing comments Signed-off-by: Yanjun Xiang --- source/common/protobuf/utility.cc | 35 +++++++++++++------------------ 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 8fa5db69ae7e..aeaa8c0f25ad 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -645,25 +645,21 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector& v namespace { -void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { - if (duration.seconds() < 0 || duration.nanos() < 0) { - throw DurationUtil::OutOfRangeException( - fmt::format("Expected positive duration: {}", duration.DebugString())); - } - if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { - throw DurationUtil::OutOfRangeException( - fmt::format("Duration out-of-range: {}", duration.DebugString())); - } -} - -bool validateDurationErrorNoThrow(const ProtobufWkt::Duration& duration, - int64_t max_seconds_value) { +absl::Status validateDurationNoThrow(const ProtobufWkt::Duration& duration, + int64_t max_seconds_value) { if (duration.seconds() < 0 || duration.nanos() < 0 || duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { ENVOY_LOG_MISC(warn, "Duration out-of-range: {}", duration.DebugString()); - return true; + return absl::OutOfRangeError("Duration out-of-range"); + } + return absl::OkStatus(); +} + +void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { + if (!validateDurationNoThrow(duration, max_seconds_value).ok()) { + throw DurationUtil::OutOfRangeException( + fmt::format("Duration out-of-range: {}", duration.DebugString())); } - return false; } void validateDuration(const ProtobufWkt::Duration& duration) { @@ -679,13 +675,10 @@ void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) { validateDuration(duration, kMaxInt64Nanoseconds); } -bool validateDurationAsMillisecondsErrorNoThrow(const ProtobufWkt::Duration& duration) { - // Apply stricter max boundary to the `seconds` value to avoid overflow. - // Note that protobuf internally converts to nanoseconds. - // The kMaxInt64Nanoseconds = 9223372036, which is about 300 years. +absl::Status validateDurationAsMillisecondsNoThrow(const ProtobufWkt::Duration& duration) { constexpr int64_t kMaxInt64Nanoseconds = std::numeric_limits::max() / (1000 * 1000 * 1000); - return validateDurationErrorNoThrow(duration, kMaxInt64Nanoseconds); + return validateDurationNoThrow(duration, kMaxInt64Nanoseconds); } } // namespace @@ -697,7 +690,7 @@ uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& durat uint64_t DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, bool& error) { - if ((error = validateDurationAsMillisecondsErrorNoThrow(duration))) { + if ((error = !validateDurationAsMillisecondsNoThrow(duration).ok())) { return 0; } return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); From 1b251222cd8a27da4cdd0e023e764b6b7b873143 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 12 May 2023 03:27:33 +0000 Subject: [PATCH 7/7] addressing comments Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/v3/ext_proc.proto | 13 ++++--- source/common/protobuf/utility.cc | 25 +++++++------ source/common/protobuf/utility.h | 8 ++-- .../filters/http/ext_proc/ext_proc.cc | 11 +++--- test/common/protobuf/utility_test.cc | 37 +++++++------------ 5 files changed, 43 insertions(+), 51 deletions(-) diff --git a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto index 2abb7bfd2df6..a924b7625922 100644 --- a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto +++ b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto @@ -139,12 +139,12 @@ message ExternalProcessor { repeated string response_attributes = 6; // Specifies the timeout for each individual message sent on the stream and - // when the filter is running in synchronous mode. Whenever - // the proxy sends a message on the stream that requires a response, it will - // reset this timer, and will stop processing and return an error (subject - // to the processing mode) if the timer expires before a matching response - // is received. There is no timeout when the filter is running in asynchronous - // mode. Default is 200 milliseconds. + // when the filter is running in synchronous mode. Whenever the proxy sends + // a message on the stream that requires a response, it will reset this timer, + // and will stop processing and return an error (subject to the processing mode) + // if the timer expires before a matching response is received. There is no + // timeout when the filter is running in asynchronous mode. The + // ``message_timeout`` range is >= 0s and <= 3600s. Default is 200 milliseconds. google.protobuf.Duration message_timeout = 7 [(validate.rules).duration = { lte {seconds: 3600} gte {} @@ -169,6 +169,7 @@ message ExternalProcessor { // Specify the upper bound of // :ref:`override_message_timeout ` + // The ``max_message_timeout`` range is >= 0s and <= 3600s. // If not specified, by default it is 0, which will effectively disable the ``override_message_timeout`` API. google.protobuf.Duration max_message_timeout = 10 [(validate.rules).duration = { lte {seconds: 3600} diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index aeaa8c0f25ad..ec8a55a36efb 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -647,18 +647,20 @@ namespace { absl::Status validateDurationNoThrow(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { - if (duration.seconds() < 0 || duration.nanos() < 0 || duration.nanos() > 999999999 || - duration.seconds() > max_seconds_value) { - ENVOY_LOG_MISC(warn, "Duration out-of-range: {}", duration.DebugString()); - return absl::OutOfRangeError("Duration out-of-range"); + if (duration.seconds() < 0 || duration.nanos() < 0) { + return absl::OutOfRangeError( + fmt::format("Expected positive duration: {}", duration.DebugString())); + } + if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { + return absl::OutOfRangeError(fmt::format("Duration out-of-range: {}", duration.DebugString())); } return absl::OkStatus(); } void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { - if (!validateDurationNoThrow(duration, max_seconds_value).ok()) { - throw DurationUtil::OutOfRangeException( - fmt::format("Duration out-of-range: {}", duration.DebugString())); + const auto result = validateDurationNoThrow(duration, max_seconds_value); + if (!result.ok()) { + throw DurationUtil::OutOfRangeException(std::string(result.message())); } } @@ -688,10 +690,11 @@ uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& durat return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); } -uint64_t DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, - bool& error) { - if ((error = !validateDurationAsMillisecondsNoThrow(duration).ok())) { - return 0; +absl::StatusOr +DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration) { + const auto result = validateDurationAsMillisecondsNoThrow(duration); + if (!result.ok()) { + return result; } return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 080abc2ecd3b..ad746301d01e 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -15,6 +15,7 @@ #include "source/common/singleton/const_singleton.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_join.h" // Obtain the value of a wrapped field (e.g. google.protobuf.UInt32Value) if set. Otherwise, return @@ -685,12 +686,11 @@ class DurationUtil { /** * Same as DurationUtil::durationToMilliseconds but does not throw an exception. - * Instead, it returns error if duration is out-of-range. * @param duration protobuf. - * @param error bool, set into true in case of duration out-of-range; otherwise false. - * @return duration in milliseconds. + * @return duration in milliseconds or an error status. */ - static uint64_t durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, bool& error); + static absl::StatusOr + durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration); /** * Same as Protobuf::util::TimeUtil::DurationToSeconds but with extra validation logic. diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index a538bcc67054..34c4ce953a9d 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -528,18 +528,17 @@ void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers } void Filter::onNewTimeout(const ProtobufWkt::Duration& override_message_timeout) { - bool error = false; - uint32_t message_timeout_ms = - DurationUtil::durationToMillisecondsNoThrow(override_message_timeout, error); - if (error) { + const auto result = DurationUtil::durationToMillisecondsNoThrow(override_message_timeout); + if (!result.ok()) { ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of duration range. " "Ignoring the message."); stats_.override_message_timeout_ignored_.inc(); return; } + const auto message_timeout_ms = result.value(); // The new timeout has to be >=1ms and <= max_message_timeout configured in filter. - const uint32_t min_timeout_ms = 1; - const uint32_t max_timeout_ms = config_->maxMessageTimeout(); + const uint64_t min_timeout_ms = 1; + const uint64_t max_timeout_ms = config_->maxMessageTimeout(); if (message_timeout_ms < min_timeout_ms || message_timeout_ms > max_timeout_ms) { ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of config range. " "Ignoring the message."); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 308af7a6bac8..4e2edbf3e687 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1711,54 +1711,43 @@ TEST(DurationUtilTest, NoThrow) { ProtobufWkt::Duration duration; duration.set_seconds(5); duration.set_nanos(10000000); - bool error = false; - auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); - EXPECT_FALSE(error); - EXPECT_TRUE(ms == 5010); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_TRUE(result.ok()); + EXPECT_TRUE(result.value() == 5010); } // Below are out-of-range tests { ProtobufWkt::Duration duration; duration.set_seconds(-1); - bool error = false; - auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); - EXPECT_TRUE(error); - EXPECT_TRUE(ms == 0); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); } { ProtobufWkt::Duration duration; duration.set_nanos(-1); - bool error = false; - auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); - EXPECT_TRUE(error); - EXPECT_TRUE(ms == 0); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); } { ProtobufWkt::Duration duration; duration.set_nanos(1000000000); - bool error = false; - auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); - EXPECT_TRUE(error); - EXPECT_TRUE(ms == 0); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); } { ProtobufWkt::Duration duration; duration.set_seconds(Protobuf::util::TimeUtil::kDurationMaxSeconds + 1); - bool error = false; - auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); - EXPECT_TRUE(error); - EXPECT_TRUE(ms == 0); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); } { ProtobufWkt::Duration duration; constexpr int64_t kMaxInt64Nanoseconds = std::numeric_limits::max() / (1000 * 1000 * 1000); duration.set_seconds(kMaxInt64Nanoseconds + 1); - bool error = false; - auto ms = DurationUtil::durationToMillisecondsNoThrow(duration, error); - EXPECT_TRUE(error); - EXPECT_TRUE(ms == 0); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); } }