config: fix crash when type URL doesn't match proto. (envoyproxy#14031)

Fixes envoyproxy#13681.

Risk level: Low
Testing: Unit and integration regression tests added.

Signed-off-by: Harvey Tuch <htuch@google.com>

source/common/config/version_converter.cc

@@ -78,20 +78,28 @@ void VersionConverter::annotateWithOriginalType(const Protobuf::Descriptor& prev
     void onMessage(Protobuf::Message& message, const void* ctxt) override {
       const Protobuf::Descriptor* descriptor = message.GetDescriptor();
       const Protobuf::Reflection* reflection = message.GetReflection();
-      const Protobuf::Descriptor& prev_descriptor = *static_cast<const Protobuf::Descriptor*>(ctxt);
+      const Protobuf::Descriptor* prev_descriptor = static_cast<const Protobuf::Descriptor*>(ctxt);
+      // If there is no previous descriptor for this message, we don't need to annotate anything.
+      if (prev_descriptor == nullptr) {
+        return;
+      }
       // If they are the same type, there's no possibility of any different type
       // further down, so we're done.
-      if (descriptor->full_name() == prev_descriptor.full_name()) {
+      if (descriptor->full_name() == prev_descriptor->full_name()) {
         return;
       }
       auto* unknown_field_set = reflection->MutableUnknownFields(&message);
       unknown_field_set->AddLengthDelimited(ProtobufWellKnown::OriginalTypeFieldNumber,
-                                            prev_descriptor.full_name());
+                                            prev_descriptor->full_name());
     }
 
     const void* onField(Protobuf::Message&, const Protobuf::FieldDescriptor& field,
                         const void* ctxt) override {
-      const Protobuf::Descriptor& prev_descriptor = *static_cast<const Protobuf::Descriptor*>(ctxt);
+      const Protobuf::Descriptor* prev_descriptor = static_cast<const Protobuf::Descriptor*>(ctxt);
+      // If there is no previous descriptor for this field, we don't need to annotate anything.
+      if (prev_descriptor == nullptr) {
+        return nullptr;
+      }
       // TODO(htuch): This is a terrible hack, there should be no per-resource
       // business logic in this file. The reason this is required is that
       // endpoints, when captured in configuration such as inlined hosts in
@@ -101,13 +109,13 @@ void VersionConverter::annotateWithOriginalType(const Protobuf::Descriptor& prev
       // In theory, we should be able to just clean up these annotations in
       // ClusterManagerImpl with type erasure, but protobuf doesn't free up memory
       // as expected, we probably need some arena level trick to address this.
-      if (prev_descriptor.full_name() == "envoy.api.v2.Cluster" &&
+      if (prev_descriptor->full_name() == "envoy.api.v2.Cluster" &&
           (field.name() == "hidden_envoy_deprecated_hosts" || field.name() == "load_assignment")) {
         // This will cause the sub-message visit to abort early.
         return field.message_type();
       }
       const Protobuf::FieldDescriptor* prev_field =
-          prev_descriptor.FindFieldByNumber(field.number());
+          prev_descriptor->FindFieldByNumber(field.number());
       return prev_field != nullptr ? prev_field->message_type() : nullptr;
     }
   };

test/common/config/version_converter_test.cc

@@ -71,6 +71,18 @@ TEST(VersionConverterProto, UpgradeNextVersion) {
   VersionConverter::upgrade(source, dst);
 }
 
+// Validate that even if we pass in a newer proto version that is being passed off as an older
+// version (e.g. via a type URL mistake), we don't crash. This is a regression test for
+// https://github.com/envoyproxy/envoy/issues/13681.
+TEST(VersionConverterProto, UpgradeWithConfusedTypes) {
+  test::common::config::NextVersion source_next;
+  source_next.mutable_new_message_in_this_version();
+  test::common::config::PreviousVersion source;
+  ASSERT_TRUE(source.ParseFromString(source_next.SerializeAsString()));
+  test::common::config::NextVersion dst;
+  VersionConverter::upgrade(source, dst);
+}
+
 // Bad UTF-8 can fail wire cast during upgrade.
 TEST(VersionConverterTest, UpgradeException) {
   API_NO_BOOST(envoy::api::v2::Cluster) source;

test/integration/ads_integration_test.cc

@@ -1521,4 +1521,21 @@ TEST_P(AdsClusterV2Test, XdsBatching) {
   initialize();
 }
 
+// Regression test for https://github.com/envoyproxy/envoy/issues/13681.
+TEST_P(AdsClusterV2Test, TypeUrlAnnotationRegression) {
+  initialize();
+  const auto cds_type_url = Config::getTypeUrl<envoy::config::cluster::v3::Cluster>(
+      envoy::config::core::v3::ApiVersion::V2);
+
+  EXPECT_TRUE(compareDiscoveryRequest(cds_type_url, "", {}, {}, {}, true));
+  auto cluster = buildCluster("cluster_0");
+  auto* bias = cluster.mutable_least_request_lb_config()->mutable_active_request_bias();
+  bias->set_default_value(1.1);
+  bias->set_runtime_key("foo");
+  sendDiscoveryResponse<envoy::config::cluster::v3::Cluster>(cds_type_url, {cluster}, {cluster}, {},
+                                                             "1", true);
+
+  test_server_->waitForCounterGe("cluster_manager.cds.update_rejected", 1);
+}
+
 } // namespace Envoy