From b0b90adf15a74da61bd6e62dbecf46632d0f01b7 Mon Sep 17 00:00:00 2001
From: htuch <htuch@users.noreply.github.com>
Date: Mon, 16 Nov 2020 18:16:22 -0500
Subject: [PATCH] config: fix crash when type URL doesn't match proto. (#14031)
Fixes #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 | 20 ++++++++++++++------
test/common/config/version_converter_test.cc | 12 ++++++++++++
test/integration/ads_integration_test.cc | 17 +++++++++++++++++
3 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc
index db2bd1cfc216..0477c29bcf86 100644
--- a/source/common/config/version_converter.cc
+++ b/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;
}
};
diff --git a/test/common/config/version_converter_test.cc b/test/common/config/version_converter_test.cc
index 65bb66145fc7..1c7e949fb625 100644
--- a/test/common/config/version_converter_test.cc
+++ b/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;
diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc
index a9d96e44a8a2..24468b45f20f 100644
--- a/test/integration/ads_integration_test.cc
+++ b/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