regex: removing exceptions (envoyproxy#37264)

Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
fredyw · Nov 21, 2024 · e0c9f1e · e0c9f1e
1 parent bb1d679
commit e0c9f1e
Show file tree

Hide file tree

Showing 22 changed files with 180 additions and 87 deletions.
diff --git a/contrib/hyperscan/regex_engines/test/integration_test.cc b/contrib/hyperscan/regex_engines/test/integration_test.cc
@@ -37,7 +37,9 @@ TEST_P(HyperscanRegexEngineIntegrationTest, Hyperscan) {
   envoy::type::matcher::v3::RegexMatcher matcher;
   *matcher.mutable_regex() = "^/asdf/.+";
 
-  EXPECT_NO_THROW(Envoy::Regex::Utility::parseRegex(matcher, test_server_->server().regexEngine()));
+  EXPECT_TRUE(Envoy::Regex::Utility::parseRegex(matcher, test_server_->server().regexEngine())
+                  .status()
+                  .ok());
 };
 
 } // namespace Hyperscan

diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h
@@ -100,7 +100,9 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher {
       if (matcher.ignore_case()) {
         ExceptionUtil::throwEnvoyException("ignore_case has no effect for safe_regex.");
       }
-      regex_ = Regex::Utility::parseRegex(matcher_.safe_regex(), context.regexEngine());
+      regex_ = THROW_OR_RETURN_VALUE(
+          Regex::Utility::parseRegex(matcher_.safe_regex(), context.regexEngine()),
+          Regex::CompiledMatcherPtr);
     } else if (matcher.match_pattern_case() == StringMatcherType::MatchPatternCase::kContains) {
       if (matcher_.ignore_case()) {
         // Cache the lowercase conversion of the Contains matcher for future use

diff --git a/source/common/common/regex.h b/source/common/common/regex.h
@@ -86,14 +86,14 @@ enum class Type { Re2, StdRegex };
 class Utility {
 public:
   template <class RegexMatcherType>
-  static CompiledMatcherPtr parseRegex(const RegexMatcherType& matcher, Engine& engine) {
+  static absl::StatusOr<CompiledMatcherPtr> parseRegex(const RegexMatcherType& matcher,
+                                                       Engine& engine) {
     // Fallback deprecated engine type in regex matcher.
     if (matcher.has_google_re2()) {
-      return THROW_OR_RETURN_VALUE(CompiledGoogleReMatcher::create(matcher),
-                                   std::unique_ptr<CompiledGoogleReMatcher>);
+      return CompiledGoogleReMatcher::create(matcher);
     }
 
-    return THROW_OR_RETURN_VALUE(engine.matcher(matcher.regex()), CompiledMatcherPtr);
+    return engine.matcher(matcher.regex());
   }
 };
 

diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc
@@ -124,12 +124,14 @@ AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCal
                              StreamInfo::FilterState::LifeSpan::FilterChain)),
       tracing_config_(Tracing::EgressConfig::get()), local_reply_(*parent.local_reply_),
       retry_policy_(createRetryPolicy(parent, options, parent_.factory_context_, creation_status)),
-      route_(std::make_shared<NullRouteImpl>(
-          parent_.cluster_->name(),
-          retry_policy_ != nullptr ? *retry_policy_ : *options.parsed_retry_policy,
-          parent_.factory_context_.regexEngine(), options.timeout, options.hash_policy)),
       account_(options.account_), buffer_limit_(options.buffer_limit_), send_xff_(options.send_xff),
       send_internal_(options.send_internal) {
+  auto route_or_error = NullRouteImpl::create(
+      parent_.cluster_->name(),
+      retry_policy_ != nullptr ? *retry_policy_ : *options.parsed_retry_policy,
+      parent_.factory_context_.regexEngine(), options.timeout, options.hash_policy);
+  SET_AND_RETURN_IF_NOT_OK(route_or_error.status(), creation_status);
+  route_ = std::move(*route_or_error);
   stream_info_.dynamicMetadata().MergeFrom(options.metadata);
   stream_info_.setIsShadow(options.is_shadow);
   stream_info_.setUpstreamClusterInfo(parent_.cluster_);

diff --git a/source/common/http/hash_policy.cc b/source/common/http/hash_policy.cc
@@ -28,11 +28,13 @@ class HashMethodImplBase : public HashPolicyImpl::HashMethod {
 class HeaderHashMethod : public HashMethodImplBase {
 public:
   HeaderHashMethod(const envoy::config::route::v3::RouteAction::HashPolicy::Header& header,
-                   bool terminal, Regex::Engine& regex_engine)
+                   bool terminal, Regex::Engine& regex_engine, absl::Status& creation_status)
       : HashMethodImplBase(terminal), header_name_(header.header_name()) {
     if (header.has_regex_rewrite()) {
       const auto& rewrite_spec = header.regex_rewrite();
-      regex_rewrite_ = Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine);
+      auto regex_or_error = Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine);
+      SET_AND_RETURN_IF_NOT_OK(regex_or_error.status(), creation_status);
+      regex_rewrite_ = std::move(*regex_or_error);
       regex_rewrite_substitution_ = rewrite_spec.substitution();
     }
   }
@@ -183,16 +185,29 @@ class FilterStateHashMethod : public HashMethodImplBase {
   const std::string key_;
 };
 
+absl::StatusOr<std::unique_ptr<HashPolicyImpl>> HashPolicyImpl::create(
+    absl::Span<const envoy::config::route::v3::RouteAction::HashPolicy* const> hash_policy,
+    Regex::Engine& regex_engine) {
+  absl::Status creation_status = absl::OkStatus();
+  std::unique_ptr<HashPolicyImpl> ret = std::unique_ptr<HashPolicyImpl>(
+      new HashPolicyImpl(hash_policy, regex_engine, creation_status));
+  RETURN_IF_NOT_OK(creation_status);
+  return ret;
+}
+
 HashPolicyImpl::HashPolicyImpl(
     absl::Span<const envoy::config::route::v3::RouteAction::HashPolicy* const> hash_policies,
-    Regex::Engine& regex_engine) {
+    Regex::Engine& regex_engine, absl::Status& creation_status) {
 
   hash_impls_.reserve(hash_policies.size());
   for (auto* hash_policy : hash_policies) {
     switch (hash_policy->policy_specifier_case()) {
     case envoy::config::route::v3::RouteAction::HashPolicy::PolicySpecifierCase::kHeader:
-      hash_impls_.emplace_back(
-          new HeaderHashMethod(hash_policy->header(), hash_policy->terminal(), regex_engine));
+      hash_impls_.emplace_back(new HeaderHashMethod(hash_policy->header(), hash_policy->terminal(),
+                                                    regex_engine, creation_status));
+      if (!creation_status.ok()) {
+        return;
+      }
       break;
     case envoy::config::route::v3::RouteAction::HashPolicy::PolicySpecifierCase::kCookie: {
       absl::optional<std::chrono::seconds> ttl;

diff --git a/source/common/http/hash_policy.h b/source/common/http/hash_policy.h
@@ -17,9 +17,9 @@ namespace Http {
  */
 class HashPolicyImpl : public HashPolicy {
 public:
-  explicit HashPolicyImpl(
-      absl::Span<const envoy::config::route::v3::RouteAction::HashPolicy* const> hash_policy,
-      Regex::Engine& regex_engine);
+  static absl::StatusOr<std::unique_ptr<HashPolicyImpl>>
+  create(absl::Span<const envoy::config::route::v3::RouteAction::HashPolicy* const> hash_policy,
+         Regex::Engine& regex_engine);
 
   // Http::HashPolicy
   absl::optional<uint64_t>
@@ -41,6 +41,11 @@ class HashPolicyImpl : public HashPolicy {
 
   using HashMethodPtr = std::unique_ptr<HashMethod>;
 
+protected:
+  explicit HashPolicyImpl(
+      absl::Span<const envoy::config::route::v3::RouteAction::HashPolicy* const> hash_policy,
+      Regex::Engine& regex_engine, absl::Status& creation_status);
+
 private:
   std::vector<HashMethodPtr> hash_impls_;
 };

diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc
@@ -94,7 +94,8 @@ HeaderUtility::createHeaderData(const envoy::config::route::v3::HeaderMatcher& c
     return std::make_unique<HeaderDataExactMatch>(config);
     break;
   case envoy::config::route::v3::HeaderMatcher::HeaderMatchSpecifierCase::kSafeRegexMatch:
-    return std::make_unique<HeaderDataRegexMatch>(config, factory_context);
+    return THROW_OR_RETURN_VALUE(HeaderDataRegexMatch::create(config, factory_context),
+                                 std::unique_ptr<HeaderDataRegexMatch>);
     break;
   case envoy::config::route::v3::HeaderMatcher::HeaderMatchSpecifierCase::kRangeMatch:
     return std::make_unique<HeaderDataRangeMatch>(config);

diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h
@@ -149,11 +149,20 @@ class HeaderUtility {
   // Corresponds to the safe_regex_match from the HeaderMatchSpecifier proto in the RDS API.
   class HeaderDataRegexMatch : public HeaderDataBaseImpl {
   public:
+    static absl::StatusOr<std::unique_ptr<HeaderDataRegexMatch>>
+    create(const envoy::config::route::v3::HeaderMatcher& config,
+           Server::Configuration::CommonFactoryContext& factory_context) {
+      auto regex_or_error =
+          Regex::Utility::parseRegex(config.safe_regex_match(), factory_context.regexEngine());
+      RETURN_IF_NOT_OK_REF(regex_or_error.status());
+      return std::unique_ptr<HeaderDataRegexMatch>(
+          new HeaderDataRegexMatch(config, std::move(*regex_or_error)));
+    }
+
+  protected:
     HeaderDataRegexMatch(const envoy::config::route::v3::HeaderMatcher& config,
-                         Server::Configuration::CommonFactoryContext& factory_context)
-        : HeaderDataBaseImpl(config),
-          regex_(Regex::Utility::parseRegex(config.safe_regex_match(),
-                                            factory_context.regexEngine())) {}
+                         Regex::CompiledMatcherPtr&& regex)
+        : HeaderDataBaseImpl(config), regex_(std::move(regex)) {}
 
   private:
     bool specificMatchesHeaders(absl::string_view header_value) const override {

diff --git a/source/common/http/null_route_impl.h b/source/common/http/null_route_impl.h
@@ -90,14 +90,30 @@ struct NullPathMatchCriterion : public Router::PathMatchCriterion {
 };
 
 struct RouteEntryImpl : public Router::RouteEntry {
+  static absl::StatusOr<std::unique_ptr<RouteEntryImpl>>
+  create(const std::string& cluster_name, const absl::optional<std::chrono::milliseconds>& timeout,
+         const Protobuf::RepeatedPtrField<envoy::config::route::v3::RouteAction::HashPolicy>&
+             hash_policy,
+         const Router::RetryPolicy& retry_policy, Regex::Engine& regex_engine) {
+    absl::Status creation_status = absl::OkStatus();
+    auto ret = std::unique_ptr<RouteEntryImpl>(new RouteEntryImpl(
+        cluster_name, timeout, hash_policy, retry_policy, regex_engine, creation_status));
+    RETURN_IF_NOT_OK(creation_status);
+    return ret;
+  }
+
+protected:
   RouteEntryImpl(
       const std::string& cluster_name, const absl::optional<std::chrono::milliseconds>& timeout,
       const Protobuf::RepeatedPtrField<envoy::config::route::v3::RouteAction::HashPolicy>&
           hash_policy,
-      const Router::RetryPolicy& retry_policy, Regex::Engine& regex_engine)
+      const Router::RetryPolicy& retry_policy, Regex::Engine& regex_engine,
+      absl::Status& creation_status)
       : retry_policy_(retry_policy), cluster_name_(cluster_name), timeout_(timeout) {
     if (!hash_policy.empty()) {
-      hash_policy_ = std::make_unique<HashPolicyImpl>(hash_policy, regex_engine);
+      auto policy_or_error = HashPolicyImpl::create(hash_policy, regex_engine);
+      SET_AND_RETURN_IF_NOT_OK(policy_or_error.status(), creation_status);
+      hash_policy_ = std::move(*policy_or_error);
     }
   }
 
@@ -212,16 +228,21 @@ struct RouteEntryImpl : public Router::RouteEntry {
 };
 
 struct NullRouteImpl : public Router::Route {
-  NullRouteImpl(const std::string cluster_name, const Router::RetryPolicy& retry_policy,
-                Regex::Engine& regex_engine,
-                const absl::optional<std::chrono::milliseconds>& timeout = {},
-                const Protobuf::RepeatedPtrField<envoy::config::route::v3::RouteAction::HashPolicy>&
-                    hash_policy = {})
-      : route_entry_(cluster_name, timeout, hash_policy, retry_policy, regex_engine) {}
+  static absl::StatusOr<std::unique_ptr<NullRouteImpl>>
+  create(const std::string cluster_name, const Router::RetryPolicy& retry_policy,
+         Regex::Engine& regex_engine, const absl::optional<std::chrono::milliseconds>& timeout = {},
+         const Protobuf::RepeatedPtrField<envoy::config::route::v3::RouteAction::HashPolicy>&
+             hash_policy = {}) {
+    absl::Status creation_status;
+    auto ret = std::unique_ptr<NullRouteImpl>(new NullRouteImpl(
+        cluster_name, retry_policy, regex_engine, timeout, hash_policy, creation_status));
+    RETURN_IF_NOT_OK(creation_status);
+    return ret;
+  }
 
   // Router::Route
   const Router::DirectResponseEntry* directResponseEntry() const override { return nullptr; }
-  const Router::RouteEntry* routeEntry() const override { return &route_entry_; }
+  const Router::RouteEntry* routeEntry() const override { return route_entry_.get(); }
   const Router::Decorator* decorator() const override { return nullptr; }
   const Router::RouteTracing* tracingConfig() const override { return nullptr; }
   const Router::RouteSpecificFilterConfig*
@@ -241,8 +262,21 @@ struct NullRouteImpl : public Router::Route {
   const std::string& routeName() const override { return EMPTY_STRING; }
   const Router::VirtualHost& virtualHost() const override { return virtual_host_; }
 
-  RouteEntryImpl route_entry_;
+  std::unique_ptr<RouteEntryImpl> route_entry_;
   static const NullVirtualHost virtual_host_;
+
+protected:
+  NullRouteImpl(const std::string cluster_name, const Router::RetryPolicy& retry_policy,
+                Regex::Engine& regex_engine,
+                const absl::optional<std::chrono::milliseconds>& timeout,
+                const Protobuf::RepeatedPtrField<envoy::config::route::v3::RouteAction::HashPolicy>&
+                    hash_policy,
+                absl::Status& creation_status) {
+    auto entry_or_error =
+        RouteEntryImpl::create(cluster_name, timeout, hash_policy, retry_policy, regex_engine);
+    SET_AND_RETURN_IF_NOT_OK(entry_or_error.status(), creation_status);
+    route_entry_ = std::move(*entry_or_error);
+  }
 };
 
 } // namespace Http

diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -230,7 +230,9 @@ createRedirectConfig(const envoy::config::route::v3::Route& route, Regex::Engine
       route.redirect().prefix_rewrite(),
       route.redirect().has_regex_rewrite() ? route.redirect().regex_rewrite().substitution() : "",
       route.redirect().has_regex_rewrite()
-          ? Regex::Utility::parseRegex(route.redirect().regex_rewrite().pattern(), regex_engine)
+          ? THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(
+                                      route.redirect().regex_rewrite().pattern(), regex_engine),
+                                  Regex::CompiledMatcherPtr)
           : nullptr,
       route.redirect().path_redirect().find('?') != absl::string_view::npos,
       route.redirect().https_redirect(),
@@ -558,8 +560,10 @@ RouteEntryImplBase::RouteEntryImplBase(const CommonVirtualHostSharedPtr& vhost,
                                     : absl::nullopt),
       host_rewrite_path_regex_(
           route.route().has_host_rewrite_path_regex()
-              ? Regex::Utility::parseRegex(route.route().host_rewrite_path_regex().pattern(),
-                                           factory_context.regexEngine())
+              ? THROW_OR_RETURN_VALUE(
+                    Regex::Utility::parseRegex(route.route().host_rewrite_path_regex().pattern(),
+                                               factory_context.regexEngine()),
+                    Regex::CompiledMatcherPtr)
               : nullptr),
       host_rewrite_path_regex_substitution_(
           route.route().has_host_rewrite_path_regex()
@@ -712,8 +716,9 @@ RouteEntryImplBase::RouteEntryImplBase(const CommonVirtualHostSharedPtr& vhost,
   }
 
   if (!route.route().hash_policy().empty()) {
-    hash_policy_ = std::make_unique<Http::HashPolicyImpl>(route.route().hash_policy(),
-                                                          factory_context.regexEngine());
+    hash_policy_ = THROW_OR_RETURN_VALUE(
+        Http::HashPolicyImpl::create(route.route().hash_policy(), factory_context.regexEngine()),
+        std::unique_ptr<Http::HashPolicyImpl>);
   }
 
   if (route.match().has_tls_context()) {
@@ -791,8 +796,9 @@ RouteEntryImplBase::RouteEntryImplBase(const CommonVirtualHostSharedPtr& vhost,
 
   if (route.route().has_regex_rewrite()) {
     auto rewrite_spec = route.route().regex_rewrite();
-    regex_rewrite_ =
-        Regex::Utility::parseRegex(rewrite_spec.pattern(), factory_context.regexEngine());
+    regex_rewrite_ = THROW_OR_RETURN_VALUE(
+        Regex::Utility::parseRegex(rewrite_spec.pattern(), factory_context.regexEngine()),
+        Regex::CompiledMatcherPtr);
     regex_rewrite_substitution_ = rewrite_spec.substitution();
   }
 

diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -562,10 +562,12 @@ bool Filter::maybeTunnel(Upstream::ThreadLocalCluster& cluster) {
   if (Runtime::runtimeFeatureEnabled(
           "envoy.restart_features.upstream_http_filters_with_tcp_proxy")) {
     // TODO(vikaschoudhary16): Initialize route_ once per cluster.
-    upstream_decoder_filter_callbacks_.route_ = std::make_shared<Http::NullRouteImpl>(
-        cluster.info()->name(),
-        *std::unique_ptr<const Router::RetryPolicy>{new Router::RetryPolicyImpl()},
-        config_->regexEngine());
+    upstream_decoder_filter_callbacks_.route_ = THROW_OR_RETURN_VALUE(
+        Http::NullRouteImpl::create(
+            cluster.info()->name(),
+            *std::unique_ptr<const Router::RetryPolicy>{new Router::RetryPolicyImpl()},
+            config_->regexEngine()),
+        std::unique_ptr<Http::NullRouteImpl>);
   }
   generic_conn_pool_ = factory->createGenericConnPool(
       cluster, config_->tunnelingConfigHelper(), this, *upstream_callbacks_,

diff --git a/source/extensions/filters/common/mutation_rules/mutation_rules.cc b/source/extensions/filters/common/mutation_rules/mutation_rules.cc
@@ -39,10 +39,14 @@ Checker::Checker(const envoy::config::common::mutation_rules::v3::HeaderMutation
                  Regex::Engine& regex_engine)
     : rules_(rules) {
   if (rules.has_allow_expression()) {
-    allow_expression_ = Regex::Utility::parseRegex(rules.allow_expression(), regex_engine);
+    allow_expression_ =
+        THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(rules.allow_expression(), regex_engine),
+                              Regex::CompiledMatcherPtr);
   }
   if (rules.has_disallow_expression()) {
-    disallow_expression_ = Regex::Utility::parseRegex(rules.disallow_expression(), regex_engine);
+    disallow_expression_ =
+        THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(rules.disallow_expression(), regex_engine),
+                              Regex::CompiledMatcherPtr);
   }
 }
 

diff --git a/source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc b/source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc
@@ -84,7 +84,9 @@ Rule::Rule(const ProtoRule& rule, Regex::Engine& regex_engine) : rule_(rule) {
 
   if (rule.on_header_present().has_regex_value_rewrite()) {
     const auto& rewrite_spec = rule.on_header_present().regex_value_rewrite();
-    regex_rewrite_ = Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine);
+    regex_rewrite_ =
+        THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine),
+                              Regex::CompiledMatcherPtr);
     regex_rewrite_substitution_ = rewrite_spec.substitution();
   }
 }

diff --git a/source/extensions/filters/http/json_to_metadata/filter.cc b/source/extensions/filters/http/json_to_metadata/filter.cc
@@ -79,8 +79,9 @@ Regex::CompiledMatcherPtr generateAllowContentTypeRegexs(
     Regex::Engine& regex_engine) {
 
   Regex::CompiledMatcherPtr allow_content_types_regex;
-  allow_content_types_regex =
-      Regex::Utility::parseRegex(proto_allow_content_types_regex, regex_engine);
+  allow_content_types_regex = THROW_OR_RETURN_VALUE(
+      Regex::Utility::parseRegex(proto_allow_content_types_regex, regex_engine),
+      Regex::CompiledMatcherPtr);
 
   return allow_content_types_regex;
 }

diff --git a/...ions/filters/network/thrift_proxy/filters/header_to_metadata/header_to_metadata_filter.cc b/...ions/filters/network/thrift_proxy/filters/header_to_metadata/header_to_metadata_filter.cc
@@ -44,7 +44,9 @@ Rule::Rule(const ProtoRule& rule, Regex::Engine& regex_engine) : rule_(rule) {
 
   if (rule.has_on_present() && rule.on_present().has_regex_value_rewrite()) {
     const auto& rewrite_spec = rule.on_present().regex_value_rewrite();
-    regex_rewrite_ = Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine);
+    regex_rewrite_ =
+        THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine),
+                              Regex::CompiledMatcherPtr);
     regex_rewrite_substitution_ = rewrite_spec.substitution();
   }
 }

diff --git a/...ns/filters/network/thrift_proxy/filters/payload_to_metadata/payload_to_metadata_filter.cc b/...ns/filters/network/thrift_proxy/filters/payload_to_metadata/payload_to_metadata_filter.cc
@@ -43,7 +43,9 @@ Rule::Rule(const ProtoRule& rule, uint16_t rule_id, TrieSharedPtr root, Regex::E
 
   if (rule_.has_on_present() && rule_.on_present().has_regex_value_rewrite()) {
     const auto& rewrite_spec = rule_.on_present().regex_value_rewrite();
-    regex_rewrite_ = Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine);
+    regex_rewrite_ =
+        THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(rewrite_spec.pattern(), regex_engine),
+                              Regex::CompiledMatcherPtr);
     regex_rewrite_substitution_ = rewrite_spec.substitution();
   }