diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h b/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h index 2351502c8ce..430f88da924 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h @@ -24,6 +24,6 @@ // For testing purpose, this channel arg indicating xds_cluster_resolver LB // policy should use the fake DNS resolver to resolve logical dns cluster. #define GRPC_ARG_XDS_LOGICAL_DNS_CLUSTER_FAKE_RESOLVER_RESPONSE_GENERATOR \ - "grpc.internal.xds_logical_dns_cluster_fake_resolver_response_generator" + "grpc.TEST_ONLY.xds_logical_dns_cluster_fake_resolver_response_generator" #endif // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_CHANNEL_ARGS_H diff --git a/src/core/ext/filters/client_channel/subchannel_pool_interface.cc b/src/core/ext/filters/client_channel/subchannel_pool_interface.cc index 6441129f8e5..d6b12ecc74c 100644 --- a/src/core/ext/filters/client_channel/subchannel_pool_interface.cc +++ b/src/core/ext/filters/client_channel/subchannel_pool_interface.cc @@ -24,7 +24,7 @@ #include "src/core/lib/gpr/useful.h" // The subchannel pool to reuse subchannels. -#define GRPC_ARG_SUBCHANNEL_POOL "grpc.subchannel_pool" +#define GRPC_ARG_SUBCHANNEL_POOL "grpc.internal.subchannel_pool" // The subchannel key ID that is only used in test to make each key unique. #define GRPC_ARG_SUBCHANNEL_KEY_TEST_ONLY_ID "grpc.subchannel_key_test_only_id" diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.cc b/src/core/ext/transport/chttp2/client/insecure/channel_create.cc index 9a199366108..8d5674ad9ea 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.cc +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.cc @@ -88,6 +88,7 @@ grpc_channel* grpc_insecure_channel_create(const char* target, const grpc_channel_args* args, void* reserved) { grpc_core::ExecCtx exec_ctx; + args = grpc_channel_args_remove_grpc_internal(args); GRPC_API_TRACE( "grpc_insecure_channel_create(target=%s, args=%p, reserved=%p)", 3, (target, args, reserved)); @@ -103,6 +104,7 @@ grpc_channel* grpc_insecure_channel_create(const char* target, grpc_channel* channel = grpc_core::CreateChannel(target, new_args, &error); // Clean up. grpc_channel_args_destroy(new_args); + grpc_channel_args_destroy(args); if (channel == nullptr) { intptr_t integer; grpc_status_code status = GRPC_STATUS_INTERNAL; diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc index ef8db979ea7..68cd3a916ca 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc @@ -154,6 +154,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds, "reserved=%p)", 4, ((void*)creds, target, (void*)args, (void*)reserved)); GPR_ASSERT(reserved == nullptr); + args = grpc_channel_args_remove_grpc_internal(args); grpc_channel* channel = nullptr; grpc_error_handle error = GRPC_ERROR_NONE; if (creds != nullptr) { @@ -173,6 +174,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds, // Clean up. grpc_channel_args_destroy(new_args); } + grpc_channel_args_destroy(args); if (channel == nullptr) { intptr_t integer; grpc_status_code status = GRPC_STATUS_INTERNAL; diff --git a/src/core/ext/xds/xds_server_config_fetcher.cc b/src/core/ext/xds/xds_server_config_fetcher.cc index fe010cf2209..8fa334fd2f3 100644 --- a/src/core/ext/xds/xds_server_config_fetcher.cc +++ b/src/core/ext/xds/xds_server_config_fetcher.cc @@ -524,10 +524,12 @@ grpc_server_config_fetcher* grpc_server_config_fetcher_xds_create( grpc_server_xds_status_notifier notifier, const grpc_channel_args* args) { grpc_core::ApplicationCallbackExecCtx callback_exec_ctx; grpc_core::ExecCtx exec_ctx; + args = grpc_channel_args_remove_grpc_internal(args); GRPC_API_TRACE("grpc_server_config_fetcher_xds_create()", 0, ()); grpc_error_handle error = GRPC_ERROR_NONE; grpc_core::RefCountedPtr xds_client = grpc_core::XdsClient::GetOrCreate(args, &error); + grpc_channel_args_destroy(args); if (error != GRPC_ERROR_NONE) { gpr_log(GPR_ERROR, "Failed to create xds client: %s", grpc_error_std_string(error).c_str()); diff --git a/src/core/lib/channel/channel_args.cc b/src/core/lib/channel/channel_args.cc index f1a4747d80d..4cd015bb49c 100644 --- a/src/core/lib/channel/channel_args.cc +++ b/src/core/lib/channel/channel_args.cc @@ -25,6 +25,7 @@ #include +#include "absl/strings/match.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" @@ -72,6 +73,22 @@ grpc_channel_args* grpc_channel_args_copy_and_remove( num_to_remove, nullptr, 0); } +grpc_channel_args* grpc_channel_args_remove_grpc_internal( + const grpc_channel_args* src) { + if (src == nullptr) return nullptr; + // Create result. + grpc_channel_args* dst = + static_cast(gpr_malloc(sizeof(grpc_channel_args))); + dst->args = + static_cast(gpr_malloc(sizeof(grpc_arg) * src->num_args)); + dst->num_args = 0; + for (size_t i = 0; i < src->num_args; ++i) { + if (absl::StartsWith(src->args[i].key, "grpc.internal.")) continue; + dst->args[dst->num_args++] = copy_arg(&src->args[i]); + } + return dst; +} + static bool should_remove_arg(const grpc_arg* arg, const char** to_remove, size_t num_to_remove) { for (size_t i = 0; i < num_to_remove; ++i) { diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index d95ae60bcce..436e7989f84 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -41,6 +41,15 @@ grpc_channel_args* grpc_channel_args_copy_and_add(const grpc_channel_args* src, const grpc_arg* to_add, size_t num_to_add); +/** Remove any channel args prefixed with 'grpc.internal.' + * These are used for internal implementation details and are not intended to + * be exposed to users. + * Returns a new channel args instance. + * Does not take ownership of \a src. + * Should be called by any public API that receives channel args. */ +grpc_channel_args* grpc_channel_args_remove_grpc_internal( + const grpc_channel_args* src); + /** Copies the arguments in \a src except for those whose keys are in \a to_remove. */ grpc_channel_args* grpc_channel_args_copy_and_remove( diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 00ddd4f7fbb..2852e09d194 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -41,7 +41,7 @@ #include "src/core/lib/json/json.h" // Channel arg key for channelz node. -#define GRPC_ARG_CHANNELZ_CHANNEL_NODE "grpc.channelz_channel_node" +#define GRPC_ARG_CHANNELZ_CHANNEL_NODE "grpc.internal.channelz_channel_node" // Channel arg key for indicating an internal channel. #define GRPC_ARG_CHANNELZ_IS_INTERNAL_CHANNEL \ diff --git a/src/core/lib/security/security_connector/security_connector.cc b/src/core/lib/security/security_connector/security_connector.cc index fe450879926..67ce84ac800 100644 --- a/src/core/lib/security/security_connector/security_connector.cc +++ b/src/core/lib/security/security_connector/security_connector.cc @@ -88,11 +88,13 @@ int grpc_server_security_connector::server_security_connector_cmp( } static void connector_arg_destroy(void* p) { + if (p == nullptr) return; static_cast(p)->Unref(DEBUG_LOCATION, "connector_arg_destroy"); } static void* connector_arg_copy(void* p) { + if (p == nullptr) return nullptr; return static_cast(p) ->Ref(DEBUG_LOCATION, "connector_arg_copy") .release(); diff --git a/src/core/lib/surface/server.cc b/src/core/lib/surface/server.cc index afe7162f842..c6b7f96b061 100644 --- a/src/core/lib/surface/server.cc +++ b/src/core/lib/surface/server.cc @@ -1469,9 +1469,11 @@ void Server::CallData::StartTransportStreamOpBatch( grpc_server* grpc_server_create(const grpc_channel_args* args, void* reserved) { grpc_core::ExecCtx exec_ctx; + args = grpc_channel_args_remove_grpc_internal(args); GRPC_API_TRACE("grpc_server_create(%p, %p)", 2, (args, reserved)); grpc_server* c_server = new grpc_server; c_server->core_server = grpc_core::MakeOrphanable(args); + grpc_channel_args_destroy(args); return c_server; } diff --git a/test/core/end2end/fuzzers/api_fuzzer_corpus/channelz-crash b/test/core/end2end/fuzzers/api_fuzzer_corpus/channelz-crash new file mode 100644 index 00000000000..0eaa5877cf8 --- /dev/null +++ b/test/core/end2end/fuzzers/api_fuzzer_corpus/channelz-crash @@ -0,0 +1,19 @@ +actions { + create_channel { + target: "\001\000\000\000\323\273`*" + channel_args { + key: "grpc.channelz_channel_node" + resource_quota { + } + } + channel_args { + resource_quota { + } + } + channel_args { + resource_quota { + } + } + } +} + diff --git a/test/core/surface/secure_channel_create_test.cc b/test/core/surface/secure_channel_create_test.cc index a0d5a54b609..97a7337b5fd 100644 --- a/test/core/surface/secure_channel_create_test.cc +++ b/test/core/surface/secure_channel_create_test.cc @@ -44,10 +44,7 @@ void test_unknown_scheme_target(void) { } void test_security_connector_already_in_arg(void) { - grpc_arg arg; - arg.type = GRPC_ARG_POINTER; - arg.value.pointer.p = nullptr; - arg.key = const_cast(GRPC_ARG_SECURITY_CONNECTOR); + grpc_arg arg = grpc_security_connector_to_arg(nullptr); grpc_channel_args args; args.num_args = 1; args.args = &arg;