From e2f3741c7434facf2438197d9865af5679f667bc Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 19 Sep 2018 11:53:55 -0700 Subject: [PATCH 01/30] Changed podspec templates and Podfile for test --- include/grpc/impl/codegen/port_platform.h | 64 +++++++++++------------ src/objective-c/GRPCClient/GRPCCall.m | 1 + src/objective-c/tests/Podfile | 30 ++--------- templates/gRPC-Core.podspec.template | 11 ++-- templates/gRPC-ProtoRPC.podspec.template | 6 +-- templates/gRPC.podspec.template | 7 +-- 6 files changed, 43 insertions(+), 76 deletions(-) diff --git a/include/grpc/impl/codegen/port_platform.h b/include/grpc/impl/codegen/port_platform.h index 031c0c36aef..46d831395da 100644 --- a/include/grpc/impl/codegen/port_platform.h +++ b/include/grpc/impl/codegen/port_platform.h @@ -185,38 +185,39 @@ #define _BSD_SOURCE #endif #if TARGET_OS_IPHONE -#define GPR_PLATFORM_STRING "ios" -#define GPR_CPU_IPHONE 1 -#define GPR_PTHREAD_TLS 1 + #define GPR_PLATFORM_STRING "ios" + #define GPR_CPU_IPHONE 1 + #define GPR_PTHREAD_TLS 1 + #define GRPC_CFSTREAM 1 #else /* TARGET_OS_IPHONE */ -#define GPR_PLATFORM_STRING "osx" -#ifdef __MAC_OS_X_VERSION_MIN_REQUIRED -#if __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 -#define GPR_CPU_IPHONE 1 -#define GPR_PTHREAD_TLS 1 -#else /* __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 */ -#define GPR_CPU_POSIX 1 -/* TODO(vjpai): there is a reported issue in bazel build for Mac where __thread - in a header is currently not working (bazelbuild/bazel#4341). Remove - the following conditional and use GPR_GCC_TLS when that is fixed */ -#ifndef GRPC_BAZEL_BUILD -#define GPR_GCC_TLS 1 -#else /* GRPC_BAZEL_BUILD */ -#define GPR_PTHREAD_TLS 1 -#endif /* GRPC_BAZEL_BUILD */ -#define GPR_APPLE_PTHREAD_NAME 1 -#endif -#else /* __MAC_OS_X_VERSION_MIN_REQUIRED */ -#define GPR_CPU_POSIX 1 -/* TODO(vjpai): Remove the following conditional and use only GPR_GCC_TLS - when bazelbuild/bazel#4341 is fixed */ -#ifndef GRPC_BAZEL_BUILD -#define GPR_GCC_TLS 1 -#else /* GRPC_BAZEL_BUILD */ -#define GPR_PTHREAD_TLS 1 -#endif /* GRPC_BAZEL_BUILD */ -#endif -#define GPR_POSIX_CRASH_HANDLER 1 + #define GPR_PLATFORM_STRING "osx" + #ifdef __MAC_OS_X_VERSION_MIN_REQUIRED + #if __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 + #define GPR_CPU_IPHONE 1 + #define GPR_PTHREAD_TLS 1 + #else /* __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 */ + #define GPR_CPU_POSIX 1 + /* TODO(vjpai): there is a reported issue in bazel build for Mac where __thread + in a header is currently not working (bazelbuild/bazel#4341). Remove + the following conditional and use GPR_GCC_TLS when that is fixed */ + #ifndef GRPC_BAZEL_BUILD + #define GPR_GCC_TLS 1 + #else /* GRPC_BAZEL_BUILD */ + #define GPR_PTHREAD_TLS 1 + #endif /* GRPC_BAZEL_BUILD */ + #define GPR_APPLE_PTHREAD_NAME 1 + #endif + #else /* __MAC_OS_X_VERSION_MIN_REQUIRED */ + #define GPR_CPU_POSIX 1 + /* TODO(vjpai): Remove the following conditional and use only GPR_GCC_TLS + when bazelbuild/bazel#4341 is fixed */ + #ifndef GRPC_BAZEL_BUILD + #define GPR_GCC_TLS 1 + #else /* GRPC_BAZEL_BUILD */ + #define GPR_PTHREAD_TLS 1 + #endif /* GRPC_BAZEL_BUILD */ + #endif + #define GPR_POSIX_CRASH_HANDLER 1 #endif #define GPR_APPLE 1 #define GPR_GCC_ATOMIC 1 @@ -228,7 +229,6 @@ #define GPR_POSIX_SYNC 1 #define GPR_POSIX_TIME 1 #define GPR_GETPID_IN_UNISTD_H 1 -/* TODO(mxyan): Remove when CFStream becomes default */ #ifndef GRPC_CFSTREAM #define GPR_SUPPORT_CHANNELS_FROM_FD 1 #endif diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 084fbdeb491..5a6b13d75b0 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -121,6 +121,7 @@ const char *kCFStreamVarName = "grpc_cfstream"; // Guarantees the code in {} block is invoked only once. See ref at: // https://developer.apple.com/documentation/objectivec/nsobject/1418639-initialize?language=objc if (self == [GRPCCall self]) { + setenv(kCFStreamVarName, "1", 1); grpc_init(); callFlags = [NSMutableDictionary dictionary]; } diff --git a/src/objective-c/tests/Podfile b/src/objective-c/tests/Podfile index 5d2f1340daa..bc0ba2f2b75 100644 --- a/src/objective-c/tests/Podfile +++ b/src/objective-c/tests/Podfile @@ -15,6 +15,9 @@ GRPC_LOCAL_SRC = '../../..' InteropTestsLocalCleartext InteropTestsRemoteWithCronet UnitTests + InteropTestsRemoteCFStream + InteropTestsLocalSSLCFStream + InteropTestsLocalCleartextCFStream ).each do |target_name| target target_name do pod 'Protobuf', :path => "#{GRPC_LOCAL_SRC}/third_party/protobuf", :inhibit_warnings => true @@ -37,27 +40,6 @@ GRPC_LOCAL_SRC = '../../..' end end -%w( - InteropTestsRemoteCFStream - InteropTestsLocalSSLCFStream - InteropTestsLocalCleartextCFStream -).each do |target_name| - target target_name do - pod 'Protobuf', :path => "#{GRPC_LOCAL_SRC}/third_party/protobuf", :inhibit_warnings => true - - pod '!ProtoCompiler', :path => "#{GRPC_LOCAL_SRC}/src/objective-c" - pod '!ProtoCompiler-gRPCPlugin', :path => "#{GRPC_LOCAL_SRC}/src/objective-c" - - pod 'BoringSSL-GRPC', :podspec => "#{GRPC_LOCAL_SRC}/src/objective-c", :inhibit_warnings => true - - pod 'gRPC/CFStream', :path => GRPC_LOCAL_SRC - pod 'gRPC-Core/CFStream-Implementation', :path => GRPC_LOCAL_SRC - pod 'gRPC-RxLibrary', :path => GRPC_LOCAL_SRC - pod 'gRPC-ProtoRPC', :path => GRPC_LOCAL_SRC, :inhibit_warnings => true - pod 'RemoteTest', :path => "RemoteTestClient", :inhibit_warnings => true - end -end - %w( CoreCronetEnd2EndTests CronetUnitTests @@ -118,11 +100,7 @@ post_install do |installer| # GPR_UNREACHABLE_CODE causes "Control may reach end of non-void # function" warning config.build_settings['GCC_WARN_ABOUT_RETURN_TYPE'] = 'NO' - if target.name.include?('CFStream') - config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_CFSTREAM=1' - else - config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_CRONET_WITH_PACKET_COALESCING=1' - end + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_CRONET_WITH_PACKET_COALESCING=1' end end diff --git a/templates/gRPC-Core.podspec.template b/templates/gRPC-Core.podspec.template index 461e8b767ff..91726ab03da 100644 --- a/templates/gRPC-Core.podspec.template +++ b/templates/gRPC-Core.podspec.template @@ -179,19 +179,14 @@ ss.compiler_flags = '-DGRPC_SHADOW_BORINGSSL_SYMBOLS' # To save you from scrolling, this is the last part of the podspec. - ss.source_files = ${ruby_multiline_list(grpc_private_files(libs), 22)} + ss.source_files = ${ruby_multiline_list(grpc_private_files(libs) + cfstream_private_files(filegroups), 22)} - ss.private_header_files = ${ruby_multiline_list(grpc_private_headers(libs), 30)} + ss.private_header_files = ${ruby_multiline_list(grpc_private_headers(libs) + cfstream_private_headers(filegroups), 30)} end + # CFStream is now default. Leaving this subspec only for compatibility purpose. s.subspec 'CFStream-Implementation' do |ss| - ss.header_mappings_dir = '.' ss.dependency "#{s.name}/Implementation", version - ss.pod_target_xcconfig = { - 'GCC_PREPROCESSOR_DEFINITIONS' => 'GRPC_CFSTREAM=1' - } - ss.source_files = ${ruby_multiline_list(cfstream_private_files(filegroups), 22)} - ss.private_header_files = ${ruby_multiline_list(cfstream_private_headers(filegroups), 30)} end s.subspec 'Cronet-Interface' do |ss| diff --git a/templates/gRPC-ProtoRPC.podspec.template b/templates/gRPC-ProtoRPC.podspec.template index 96966784f18..cfc4b60b2eb 100644 --- a/templates/gRPC-ProtoRPC.podspec.template +++ b/templates/gRPC-ProtoRPC.podspec.template @@ -54,12 +54,10 @@ ss.source_files = "#{src_dir}/*.{h,m}" end + + # CFStream is now default. Leaving this subspec only for compatibility purpose. s.subspec 'CFStream' do |ss| - ss.dependency 'gRPC/CFStream', version ss.dependency "#{s.name}/Main", version - ss.pod_target_xcconfig = { - 'GCC_PREPROCESSOR_DEFINITIONS' => 'GRPC_CFSTREAM=1' - } end s.pod_target_xcconfig = { diff --git a/templates/gRPC.podspec.template b/templates/gRPC.podspec.template index a3190c2d8e6..c8054e1d1d0 100644 --- a/templates/gRPC.podspec.template +++ b/templates/gRPC.podspec.template @@ -65,14 +65,9 @@ ss.dependency 'gRPC-Core', version end - # This subspec is mutually exclusive with the `Main` subspec + # CFStream is now default. Leaving this subspec only for compatibility purpose. s.subspec 'CFStream' do |ss| - ss.dependency 'gRPC-Core/CFStream-Implementation', version ss.dependency "#{s.name}/Main", version - - ss.pod_target_xcconfig = { - 'GCC_PREPROCESSOR_DEFINITIONS' => 'GRPC_CFSTREAM=1' - } end s.subspec 'GID' do |ss| From acf3a72e243a37b68f4c0da7e72d61a62127ba75 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 14 Dec 2018 13:46:53 -0800 Subject: [PATCH 02/30] CFStream use serial dispatch queue --- src/core/lib/iomgr/cfstream_handle.cc | 98 ++++++++++++--------------- src/core/lib/iomgr/cfstream_handle.h | 2 + 2 files changed, 46 insertions(+), 54 deletions(-) diff --git a/src/core/lib/iomgr/cfstream_handle.cc b/src/core/lib/iomgr/cfstream_handle.cc index 827fd24831e..bb402f96cf5 100644 --- a/src/core/lib/iomgr/cfstream_handle.cc +++ b/src/core/lib/iomgr/cfstream_handle.cc @@ -52,62 +52,53 @@ CFStreamHandle* CFStreamHandle::CreateStreamHandle( void CFStreamHandle::ReadCallback(CFReadStreamRef stream, CFStreamEventType type, void* client_callback_info) { + grpc_core::ExecCtx exec_ctx; CFStreamHandle* handle = static_cast(client_callback_info); - CFSTREAM_HANDLE_REF(handle, "read callback"); - dispatch_async( - dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - grpc_core::ExecCtx exec_ctx; - if (grpc_tcp_trace.enabled()) { - gpr_log(GPR_DEBUG, "CFStream ReadCallback (%p, %p, %lu, %p)", handle, - stream, type, client_callback_info); - } - switch (type) { - case kCFStreamEventOpenCompleted: - handle->open_event_.SetReady(); - break; - case kCFStreamEventHasBytesAvailable: - case kCFStreamEventEndEncountered: - handle->read_event_.SetReady(); - break; - case kCFStreamEventErrorOccurred: - handle->open_event_.SetReady(); - handle->read_event_.SetReady(); - break; - default: - GPR_UNREACHABLE_CODE(return ); - } - CFSTREAM_HANDLE_UNREF(handle, "read callback"); - }); + if (grpc_tcp_trace.enabled()) { + gpr_log(GPR_DEBUG, "CFStream ReadCallback (%p, %p, %lu, %p)", handle, + stream, type, client_callback_info); + } + switch (type) { + case kCFStreamEventOpenCompleted: + handle->open_event_.SetReady(); + break; + case kCFStreamEventHasBytesAvailable: + case kCFStreamEventEndEncountered: + handle->read_event_.SetReady(); + break; + case kCFStreamEventErrorOccurred: + handle->open_event_.SetReady(); + handle->read_event_.SetReady(); + break; + default: + GPR_UNREACHABLE_CODE(return ); + } } void CFStreamHandle::WriteCallback(CFWriteStreamRef stream, CFStreamEventType type, void* clientCallBackInfo) { + grpc_core::ExecCtx exec_ctx; CFStreamHandle* handle = static_cast(clientCallBackInfo); - CFSTREAM_HANDLE_REF(handle, "write callback"); - dispatch_async( - dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - grpc_core::ExecCtx exec_ctx; - if (grpc_tcp_trace.enabled()) { - gpr_log(GPR_DEBUG, "CFStream WriteCallback (%p, %p, %lu, %p)", handle, - stream, type, clientCallBackInfo); - } - switch (type) { - case kCFStreamEventOpenCompleted: - handle->open_event_.SetReady(); - break; - case kCFStreamEventCanAcceptBytes: - case kCFStreamEventEndEncountered: - handle->write_event_.SetReady(); - break; - case kCFStreamEventErrorOccurred: - handle->open_event_.SetReady(); - handle->write_event_.SetReady(); - break; - default: - GPR_UNREACHABLE_CODE(return ); - } - CFSTREAM_HANDLE_UNREF(handle, "write callback"); - }); + printf("** CFStreamHandle::WriteCallback\n"); + if (grpc_tcp_trace.enabled()) { + gpr_log(GPR_DEBUG, "CFStream WriteCallback (%p, %p, %lu, %p)", handle, + stream, type, clientCallBackInfo); + } + switch (type) { + case kCFStreamEventOpenCompleted: + handle->open_event_.SetReady(); + break; + case kCFStreamEventCanAcceptBytes: + case kCFStreamEventEndEncountered: + handle->write_event_.SetReady(); + break; + case kCFStreamEventErrorOccurred: + handle->open_event_.SetReady(); + handle->write_event_.SetReady(); + break; + default: + GPR_UNREACHABLE_CODE(return ); + } } CFStreamHandle::CFStreamHandle(CFReadStreamRef read_stream, @@ -116,6 +107,7 @@ CFStreamHandle::CFStreamHandle(CFReadStreamRef read_stream, open_event_.InitEvent(); read_event_.InitEvent(); write_event_.InitEvent(); + dispatch_queue_ = dispatch_queue_create(nullptr, DISPATCH_QUEUE_SERIAL); CFStreamClientContext ctx = {0, static_cast(this), CFStreamHandle::Retain, CFStreamHandle::Release, nil}; @@ -129,10 +121,8 @@ CFStreamHandle::CFStreamHandle(CFReadStreamRef read_stream, kCFStreamEventOpenCompleted | kCFStreamEventCanAcceptBytes | kCFStreamEventErrorOccurred | kCFStreamEventEndEncountered, CFStreamHandle::WriteCallback, &ctx); - CFReadStreamScheduleWithRunLoop(read_stream, CFRunLoopGetMain(), - kCFRunLoopCommonModes); - CFWriteStreamScheduleWithRunLoop(write_stream, CFRunLoopGetMain(), - kCFRunLoopCommonModes); + CFReadStreamSetDispatchQueue(read_stream, dispatch_queue_); + CFWriteStreamSetDispatchQueue(write_stream, dispatch_queue_); } CFStreamHandle::~CFStreamHandle() { diff --git a/src/core/lib/iomgr/cfstream_handle.h b/src/core/lib/iomgr/cfstream_handle.h index 4258e72431c..93ec5f044bb 100644 --- a/src/core/lib/iomgr/cfstream_handle.h +++ b/src/core/lib/iomgr/cfstream_handle.h @@ -62,6 +62,8 @@ class CFStreamHandle final { grpc_core::LockfreeEvent read_event_; grpc_core::LockfreeEvent write_event_; + dispatch_queue_t dispatch_queue_; + gpr_refcount refcount_; }; From a7643b1f424aac7481a0f7db615cc24d36724736 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 27 Dec 2018 11:40:17 -0800 Subject: [PATCH 03/30] build projects --- gRPC-Core.podspec | 31 ++++++++++++++----------------- gRPC-ProtoRPC.podspec | 6 ++---- gRPC.podspec | 7 +------ 3 files changed, 17 insertions(+), 27 deletions(-) diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index f873bc693bc..b5b8aef2225 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -846,7 +846,15 @@ Pod::Spec.new do |s| 'src/core/ext/filters/http/client_authority_filter.cc', 'src/core/ext/filters/workarounds/workaround_cronet_compression_filter.cc', 'src/core/ext/filters/workarounds/workaround_utils.cc', - 'src/core/plugin_registry/grpc_plugin_registry.cc' + 'src/core/plugin_registry/grpc_plugin_registry.cc', + 'src/core/lib/iomgr/cfstream_handle.cc', + 'src/core/lib/iomgr/endpoint_cfstream.cc', + 'src/core/lib/iomgr/error_cfstream.cc', + 'src/core/lib/iomgr/iomgr_posix_cfstream.cc', + 'src/core/lib/iomgr/tcp_client_cfstream.cc', + 'src/core/lib/iomgr/cfstream_handle.h', + 'src/core/lib/iomgr/endpoint_cfstream.h', + 'src/core/lib/iomgr/error_cfstream.h' ss.private_header_files = 'src/core/lib/gpr/alloc.h', 'src/core/lib/gpr/arena.h', @@ -1148,26 +1156,15 @@ Pod::Spec.new do |s| 'src/core/ext/filters/message_size/message_size_filter.h', 'src/core/ext/filters/http/client_authority_filter.h', 'src/core/ext/filters/workarounds/workaround_cronet_compression_filter.h', - 'src/core/ext/filters/workarounds/workaround_utils.h' + 'src/core/ext/filters/workarounds/workaround_utils.h', + 'src/core/lib/iomgr/cfstream_handle.h', + 'src/core/lib/iomgr/endpoint_cfstream.h', + 'src/core/lib/iomgr/error_cfstream.h' end + # CFStream is now default. Leaving this subspec only for compatibility purpose. s.subspec 'CFStream-Implementation' do |ss| - ss.header_mappings_dir = '.' ss.dependency "#{s.name}/Implementation", version - ss.pod_target_xcconfig = { - 'GCC_PREPROCESSOR_DEFINITIONS' => 'GRPC_CFSTREAM=1' - } - ss.source_files = 'src/core/lib/iomgr/cfstream_handle.cc', - 'src/core/lib/iomgr/endpoint_cfstream.cc', - 'src/core/lib/iomgr/error_cfstream.cc', - 'src/core/lib/iomgr/iomgr_posix_cfstream.cc', - 'src/core/lib/iomgr/tcp_client_cfstream.cc', - 'src/core/lib/iomgr/cfstream_handle.h', - 'src/core/lib/iomgr/endpoint_cfstream.h', - 'src/core/lib/iomgr/error_cfstream.h' - ss.private_header_files = 'src/core/lib/iomgr/cfstream_handle.h', - 'src/core/lib/iomgr/endpoint_cfstream.h', - 'src/core/lib/iomgr/error_cfstream.h' end s.subspec 'Cronet-Interface' do |ss| diff --git a/gRPC-ProtoRPC.podspec b/gRPC-ProtoRPC.podspec index 13fe3e0b9c0..d27df2d30b7 100644 --- a/gRPC-ProtoRPC.podspec +++ b/gRPC-ProtoRPC.podspec @@ -52,12 +52,10 @@ Pod::Spec.new do |s| ss.source_files = "#{src_dir}/*.{h,m}" end + + # CFStream is now default. Leaving this subspec only for compatibility purpose. s.subspec 'CFStream' do |ss| - ss.dependency 'gRPC/CFStream', version ss.dependency "#{s.name}/Main", version - ss.pod_target_xcconfig = { - 'GCC_PREPROCESSOR_DEFINITIONS' => 'GRPC_CFSTREAM=1' - } end s.pod_target_xcconfig = { diff --git a/gRPC.podspec b/gRPC.podspec index 940a1ac6217..ddc32da6261 100644 --- a/gRPC.podspec +++ b/gRPC.podspec @@ -63,14 +63,9 @@ Pod::Spec.new do |s| ss.dependency 'gRPC-Core', version end - # This subspec is mutually exclusive with the `Main` subspec + # CFStream is now default. Leaving this subspec only for compatibility purpose. s.subspec 'CFStream' do |ss| - ss.dependency 'gRPC-Core/CFStream-Implementation', version ss.dependency "#{s.name}/Main", version - - ss.pod_target_xcconfig = { - 'GCC_PREPROCESSOR_DEFINITIONS' => 'GRPC_CFSTREAM=1' - } end s.subspec 'GID' do |ss| From 80f005ee8fe33c21debbe0af1e976ff96d8bf0cf Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 27 Dec 2018 13:46:52 -0800 Subject: [PATCH 04/30] Remove debug info; clang-format --- BUILD | 28 +++------- include/grpc/impl/codegen/port_platform.h | 64 +++++++++++------------ src/core/lib/iomgr/cfstream_handle.cc | 1 - 3 files changed, 40 insertions(+), 53 deletions(-) diff --git a/BUILD b/BUILD index e3c765198b2..b8abce38072 100644 --- a/BUILD +++ b/BUILD @@ -706,12 +706,15 @@ grpc_cc_library( "src/core/lib/http/parser.cc", "src/core/lib/iomgr/buffer_list.cc", "src/core/lib/iomgr/call_combiner.cc", + "src/core/lib/iomgr/cfstream_handle.cc", "src/core/lib/iomgr/combiner.cc", "src/core/lib/iomgr/endpoint.cc", + "src/core/lib/iomgr/endpoint_cfstream.cc", "src/core/lib/iomgr/endpoint_pair_posix.cc", "src/core/lib/iomgr/endpoint_pair_uv.cc", "src/core/lib/iomgr/endpoint_pair_windows.cc", "src/core/lib/iomgr/error.cc", + "src/core/lib/iomgr/error_cfstream.cc", "src/core/lib/iomgr/ev_epoll1_linux.cc", "src/core/lib/iomgr/ev_epollex_linux.cc", "src/core/lib/iomgr/ev_poll_posix.cc", @@ -730,6 +733,7 @@ grpc_cc_library( "src/core/lib/iomgr/iomgr_custom.cc", "src/core/lib/iomgr/iomgr_internal.cc", "src/core/lib/iomgr/iomgr_posix.cc", + "src/core/lib/iomgr/iomgr_posix_cfstream.cc", "src/core/lib/iomgr/iomgr_windows.cc", "src/core/lib/iomgr/is_epollexclusive_available.cc", "src/core/lib/iomgr/load_file.cc", @@ -757,6 +761,7 @@ grpc_cc_library( "src/core/lib/iomgr/socket_utils_windows.cc", "src/core/lib/iomgr/socket_windows.cc", "src/core/lib/iomgr/tcp_client.cc", + "src/core/lib/iomgr/tcp_client_cfstream.cc", "src/core/lib/iomgr/tcp_client_custom.cc", "src/core/lib/iomgr/tcp_client_posix.cc", "src/core/lib/iomgr/tcp_client_windows.cc", @@ -858,12 +863,15 @@ grpc_cc_library( "src/core/lib/iomgr/block_annotate.h", "src/core/lib/iomgr/buffer_list.h", "src/core/lib/iomgr/call_combiner.h", + "src/core/lib/iomgr/cfstream_handle.h", "src/core/lib/iomgr/closure.h", "src/core/lib/iomgr/combiner.h", "src/core/lib/iomgr/dynamic_annotations.h", "src/core/lib/iomgr/endpoint.h", + "src/core/lib/iomgr/endpoint_cfstream.h", "src/core/lib/iomgr/endpoint_pair.h", "src/core/lib/iomgr/error.h", + "src/core/lib/iomgr/error_cfstream.h", "src/core/lib/iomgr/error_internal.h", "src/core/lib/iomgr/ev_epoll1_linux.h", "src/core/lib/iomgr/ev_epollex_linux.h", @@ -1018,26 +1026,6 @@ grpc_cc_library( ], ) -grpc_cc_library( - name = "grpc_cfstream", - srcs = [ - "src/core/lib/iomgr/cfstream_handle.cc", - "src/core/lib/iomgr/endpoint_cfstream.cc", - "src/core/lib/iomgr/error_cfstream.cc", - "src/core/lib/iomgr/iomgr_posix_cfstream.cc", - "src/core/lib/iomgr/tcp_client_cfstream.cc", - ], - hdrs = [ - "src/core/lib/iomgr/cfstream_handle.h", - "src/core/lib/iomgr/endpoint_cfstream.h", - "src/core/lib/iomgr/error_cfstream.h", - ], - deps = [ - ":gpr_base", - ":grpc_base", - ], -) - grpc_cc_library( name = "grpc_client_channel", srcs = [ diff --git a/include/grpc/impl/codegen/port_platform.h b/include/grpc/impl/codegen/port_platform.h index 46d831395da..21253db01e4 100644 --- a/include/grpc/impl/codegen/port_platform.h +++ b/include/grpc/impl/codegen/port_platform.h @@ -185,39 +185,39 @@ #define _BSD_SOURCE #endif #if TARGET_OS_IPHONE - #define GPR_PLATFORM_STRING "ios" - #define GPR_CPU_IPHONE 1 - #define GPR_PTHREAD_TLS 1 - #define GRPC_CFSTREAM 1 +#define GPR_PLATFORM_STRING "ios" +#define GPR_CPU_IPHONE 1 +#define GPR_PTHREAD_TLS 1 +#define GRPC_CFSTREAM 1 #else /* TARGET_OS_IPHONE */ - #define GPR_PLATFORM_STRING "osx" - #ifdef __MAC_OS_X_VERSION_MIN_REQUIRED - #if __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 - #define GPR_CPU_IPHONE 1 - #define GPR_PTHREAD_TLS 1 - #else /* __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 */ - #define GPR_CPU_POSIX 1 - /* TODO(vjpai): there is a reported issue in bazel build for Mac where __thread - in a header is currently not working (bazelbuild/bazel#4341). Remove - the following conditional and use GPR_GCC_TLS when that is fixed */ - #ifndef GRPC_BAZEL_BUILD - #define GPR_GCC_TLS 1 - #else /* GRPC_BAZEL_BUILD */ - #define GPR_PTHREAD_TLS 1 - #endif /* GRPC_BAZEL_BUILD */ - #define GPR_APPLE_PTHREAD_NAME 1 - #endif - #else /* __MAC_OS_X_VERSION_MIN_REQUIRED */ - #define GPR_CPU_POSIX 1 - /* TODO(vjpai): Remove the following conditional and use only GPR_GCC_TLS - when bazelbuild/bazel#4341 is fixed */ - #ifndef GRPC_BAZEL_BUILD - #define GPR_GCC_TLS 1 - #else /* GRPC_BAZEL_BUILD */ - #define GPR_PTHREAD_TLS 1 - #endif /* GRPC_BAZEL_BUILD */ - #endif - #define GPR_POSIX_CRASH_HANDLER 1 +#define GPR_PLATFORM_STRING "osx" +#ifdef __MAC_OS_X_VERSION_MIN_REQUIRED +#if __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 +#define GPR_CPU_IPHONE 1 +#define GPR_PTHREAD_TLS 1 +#else /* __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_7 */ +#define GPR_CPU_POSIX 1 +/* TODO(vjpai): there is a reported issue in bazel build for Mac where __thread + in a header is currently not working (bazelbuild/bazel#4341). Remove + the following conditional and use GPR_GCC_TLS when that is fixed */ +#ifndef GRPC_BAZEL_BUILD +#define GPR_GCC_TLS 1 +#else /* GRPC_BAZEL_BUILD */ +#define GPR_PTHREAD_TLS 1 +#endif /* GRPC_BAZEL_BUILD */ +#define GPR_APPLE_PTHREAD_NAME 1 +#endif +#else /* __MAC_OS_X_VERSION_MIN_REQUIRED */ +#define GPR_CPU_POSIX 1 +/* TODO(vjpai): Remove the following conditional and use only GPR_GCC_TLS + when bazelbuild/bazel#4341 is fixed */ +#ifndef GRPC_BAZEL_BUILD +#define GPR_GCC_TLS 1 +#else /* GRPC_BAZEL_BUILD */ +#define GPR_PTHREAD_TLS 1 +#endif /* GRPC_BAZEL_BUILD */ +#endif +#define GPR_POSIX_CRASH_HANDLER 1 #endif #define GPR_APPLE 1 #define GPR_GCC_ATOMIC 1 diff --git a/src/core/lib/iomgr/cfstream_handle.cc b/src/core/lib/iomgr/cfstream_handle.cc index bb402f96cf5..6cb9ca1a0d4 100644 --- a/src/core/lib/iomgr/cfstream_handle.cc +++ b/src/core/lib/iomgr/cfstream_handle.cc @@ -79,7 +79,6 @@ void CFStreamHandle::WriteCallback(CFWriteStreamRef stream, void* clientCallBackInfo) { grpc_core::ExecCtx exec_ctx; CFStreamHandle* handle = static_cast(clientCallBackInfo); - printf("** CFStreamHandle::WriteCallback\n"); if (grpc_tcp_trace.enabled()) { gpr_log(GPR_DEBUG, "CFStream WriteCallback (%p, %p, %lu, %p)", handle, stream, type, clientCallBackInfo); From 303416080c447dae8729e1ffbe78a366f1f88a71 Mon Sep 17 00:00:00 2001 From: Thibaut Le Guilly Date: Tue, 9 Oct 2018 17:10:22 +0900 Subject: [PATCH 05/30] Provide access to verify_peer_callback from C# --- src/csharp/Grpc.Core/ChannelCredentials.cs | 69 +++++++++++++++++-- .../Internal/ChannelCredentialsSafeHandle.cs | 12 +++- .../Internal/NativeMethods.Generated.cs | 6 +- src/csharp/Grpc.Core/VerifyPeerContext.cs | 30 ++++++++ .../SslCredentialsTest.cs | 43 +++++++++++- src/csharp/ext/grpc_csharp_ext.c | 41 +++++++++-- .../Grpc.Core/Internal/native_methods.include | 2 +- 7 files changed, 184 insertions(+), 19 deletions(-) create mode 100644 src/csharp/Grpc.Core/VerifyPeerContext.cs diff --git a/src/csharp/Grpc.Core/ChannelCredentials.cs b/src/csharp/Grpc.Core/ChannelCredentials.cs index 3ce32f31b76..103a594bb06 100644 --- a/src/csharp/Grpc.Core/ChannelCredentials.cs +++ b/src/csharp/Grpc.Core/ChannelCredentials.cs @@ -18,9 +18,11 @@ using System; using System.Collections.Generic; +using System.Runtime.InteropServices; using System.Threading.Tasks; using Grpc.Core.Internal; +using Grpc.Core.Logging; using Grpc.Core.Utils; namespace Grpc.Core @@ -104,20 +106,36 @@ namespace Grpc.Core } } + /// + /// Callback invoked with the expected targetHost and the peer's certificate. + /// If false is returned by this callback then it is treated as a + /// verification failure. Invocation of the callback is blocking, so any + /// implementation should be light-weight. + /// + /// The associated with the callback + /// true if verification succeeded, false otherwise. + /// Note: experimental API that can change or be removed without any prior notice. + public delegate bool VerifyPeerCallback(VerifyPeerContext context); + /// /// Client-side SSL credentials. /// public sealed class SslCredentials : ChannelCredentials { + static readonly ILogger Logger = GrpcEnvironment.Logger.ForType(); + readonly string rootCertificates; readonly KeyCertificatePair keyCertificatePair; + readonly VerifyPeerCallback verifyPeerCallback; + readonly VerifyPeerCallbackInternal verifyPeerCallbackInternal; + readonly GCHandle gcHandle; /// /// Creates client-side SSL credentials loaded from /// disk file pointed to by the GRPC_DEFAULT_SSL_ROOTS_FILE_PATH environment variable. /// If that fails, gets the roots certificates from a well known place on disk. /// - public SslCredentials() : this(null, null) + public SslCredentials() : this(null, null, null) { } @@ -125,19 +143,37 @@ namespace Grpc.Core /// Creates client-side SSL credentials from /// a string containing PEM encoded root certificates. /// - public SslCredentials(string rootCertificates) : this(rootCertificates, null) + public SslCredentials(string rootCertificates) : this(rootCertificates, null, null) { } - + + /// + /// Creates client-side SSL credentials. + /// + /// string containing PEM encoded server root certificates. + /// a key certificate pair. + public SslCredentials(string rootCertificates, KeyCertificatePair keyCertificatePair) : + this(rootCertificates, keyCertificatePair, null) + { + } + /// /// Creates client-side SSL credentials. /// /// string containing PEM encoded server root certificates. /// a key certificate pair. - public SslCredentials(string rootCertificates, KeyCertificatePair keyCertificatePair) + /// a callback to verify peer's target name and certificate. + /// Note: experimental API that can change or be removed without any prior notice. + public SslCredentials(string rootCertificates, KeyCertificatePair keyCertificatePair, VerifyPeerCallback verifyPeerCallback) { this.rootCertificates = rootCertificates; this.keyCertificatePair = keyCertificatePair; + if (verifyPeerCallback != null) + { + this.verifyPeerCallback = verifyPeerCallback; + this.verifyPeerCallbackInternal = this.VerifyPeerCallbackHandler; + gcHandle = GCHandle.Alloc(verifyPeerCallbackInternal); + } } /// @@ -171,7 +207,30 @@ namespace Grpc.Core internal override ChannelCredentialsSafeHandle CreateNativeCredentials() { - return ChannelCredentialsSafeHandle.CreateSslCredentials(rootCertificates, keyCertificatePair); + return ChannelCredentialsSafeHandle.CreateSslCredentials(rootCertificates, keyCertificatePair, this.verifyPeerCallbackInternal); + } + + private int VerifyPeerCallbackHandler(IntPtr host, IntPtr pem, IntPtr userData, bool isDestroy) + { + if (isDestroy) + { + this.gcHandle.Free(); + return 0; + } + + try + { + var context = new VerifyPeerContext(Marshal.PtrToStringAnsi(host), Marshal.PtrToStringAnsi(pem)); + + return this.verifyPeerCallback(context) ? 0 : 1; + } + catch (Exception e) + { + // eat the exception, we must not throw when inside callback from native code. + Logger.Error(e, "Exception occurred while invoking verify peer callback handler."); + // Return validation failure in case of exception. + return 1; + } } } diff --git a/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs b/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs index 11b5d2cc3f6..5e336673153 100644 --- a/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs @@ -20,6 +20,12 @@ using System.Threading.Tasks; namespace Grpc.Core.Internal { + internal delegate int VerifyPeerCallbackInternal( + IntPtr targetHost, + IntPtr targetPem, + IntPtr userData, + bool isDestroy); + /// /// grpc_channel_credentials from grpc/grpc_security.h /// @@ -38,15 +44,15 @@ namespace Grpc.Core.Internal return creds; } - public static ChannelCredentialsSafeHandle CreateSslCredentials(string pemRootCerts, KeyCertificatePair keyCertPair) + public static ChannelCredentialsSafeHandle CreateSslCredentials(string pemRootCerts, KeyCertificatePair keyCertPair, VerifyPeerCallbackInternal verifyPeerCallback) { if (keyCertPair != null) { - return Native.grpcsharp_ssl_credentials_create(pemRootCerts, keyCertPair.CertificateChain, keyCertPair.PrivateKey); + return Native.grpcsharp_ssl_credentials_create(pemRootCerts, keyCertPair.CertificateChain, keyCertPair.PrivateKey, verifyPeerCallback); } else { - return Native.grpcsharp_ssl_credentials_create(pemRootCerts, null, null); + return Native.grpcsharp_ssl_credentials_create(pemRootCerts, null, null, verifyPeerCallback); } } diff --git a/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs b/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs index b7b9a12d8a3..e09b9813f79 100644 --- a/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs +++ b/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs @@ -482,7 +482,7 @@ namespace Grpc.Core.Internal public delegate void grpcsharp_channel_args_set_integer_delegate(ChannelArgsSafeHandle args, UIntPtr index, string key, int value); public delegate void grpcsharp_channel_args_destroy_delegate(IntPtr args); public delegate void grpcsharp_override_default_ssl_roots_delegate(string pemRootCerts); - public delegate ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create_delegate(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey); + public delegate ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create_delegate(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback); public delegate ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create_delegate(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds); public delegate void grpcsharp_channel_credentials_release_delegate(IntPtr credentials); public delegate ChannelSafeHandle grpcsharp_insecure_channel_create_delegate(string target, ChannelArgsSafeHandle channelArgs); @@ -676,7 +676,7 @@ namespace Grpc.Core.Internal public static extern void grpcsharp_override_default_ssl_roots(string pemRootCerts); [DllImport(ImportName)] - public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey); + public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback); [DllImport(ImportName)] public static extern ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds); @@ -972,7 +972,7 @@ namespace Grpc.Core.Internal public static extern void grpcsharp_override_default_ssl_roots(string pemRootCerts); [DllImport(ImportName)] - public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey); + public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback); [DllImport(ImportName)] public static extern ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds); diff --git a/src/csharp/Grpc.Core/VerifyPeerContext.cs b/src/csharp/Grpc.Core/VerifyPeerContext.cs new file mode 100644 index 00000000000..b48984b093c --- /dev/null +++ b/src/csharp/Grpc.Core/VerifyPeerContext.cs @@ -0,0 +1,30 @@ +namespace Grpc.Core +{ + /// + /// Verification context for VerifyPeerCallback. + /// Note: experimental API that can change or be removed without any prior notice. + /// + public class VerifyPeerContext + { + /// + /// Initializes a new instance of the class. + /// + /// string containing the host name of the peer. + /// string containing PEM encoded certificate of the peer. + internal VerifyPeerContext(string targetHost, string targetPem) + { + this.TargetHost = targetHost; + this.TargetPem = targetPem; + } + + /// + /// String containing the host name of the peer. + /// + public string TargetHost { get; } + + /// + /// string containing PEM encoded certificate of the peer. + /// + public string TargetPem { get; } + } +} diff --git a/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs b/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs index b3c47c2d8d3..818ac6d1adc 100644 --- a/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs +++ b/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs @@ -44,17 +44,23 @@ namespace Grpc.IntegrationTesting string rootCert; KeyCertificatePair keyCertPair; + string certChain; + List options; + bool isHostEqual; + bool isPemEqual; public void InitClientAndServer(bool clientAddKeyCertPair, SslClientCertificateRequestType clientCertRequestType) { rootCert = File.ReadAllText(TestCredentials.ClientCertAuthorityPath); + certChain = File.ReadAllText(TestCredentials.ServerCertChainPath); + certChain = certChain.Replace("\r", string.Empty); keyCertPair = new KeyCertificatePair( - File.ReadAllText(TestCredentials.ServerCertChainPath), + certChain, File.ReadAllText(TestCredentials.ServerPrivateKeyPath)); var serverCredentials = new SslServerCredentials(new[] { keyCertPair }, rootCert, clientCertRequestType); - var clientCredentials = clientAddKeyCertPair ? new SslCredentials(rootCert, keyCertPair) : new SslCredentials(rootCert); + var clientCredentials = clientAddKeyCertPair ? new SslCredentials(rootCert, keyCertPair, context => this.VerifyPeerCallback(context, true)) : new SslCredentials(rootCert); // Disable SO_REUSEPORT to prevent https://github.com/grpc/grpc/issues/10755 server = new Server(new[] { new ChannelOption(ChannelOptions.SoReuseport, 0) }) @@ -64,7 +70,7 @@ namespace Grpc.IntegrationTesting }; server.Start(); - var options = new List + options = new List { new ChannelOption(ChannelOptions.SslTargetNameOverride, TestCredentials.DefaultHostOverride) }; @@ -210,6 +216,37 @@ namespace Grpc.IntegrationTesting Assert.AreEqual(12345, response.AggregatedPayloadSize); } + [Test] + public void VerifyPeerCallbackTest() + { + InitClientAndServer(true, SslClientCertificateRequestType.RequestAndRequireAndVerify); + + // Force GC collection to verify that the VerifyPeerCallback is not collected. If + // it gets collected, this test will hang. + GC.Collect(); + + client.UnaryCall(new SimpleRequest { ResponseSize = 10 }); + Assert.IsTrue(isHostEqual); + Assert.IsTrue(isPemEqual); + } + + [Test] + public void VerifyPeerCallbackFailTest() + { + InitClientAndServer(true, SslClientCertificateRequestType.RequestAndRequireAndVerify); + var clientCredentials = new SslCredentials(rootCert, keyCertPair, context => this.VerifyPeerCallback(context, false)); + var failingChannel = new Channel(Host, server.Ports.Single().BoundPort, clientCredentials, options); + var failingClient = new TestService.TestServiceClient(failingChannel); + Assert.Throws(() => failingClient.UnaryCall(new SimpleRequest { ResponseSize = 10 })); + } + + private bool VerifyPeerCallback(VerifyPeerContext context, bool returnValue) + { + isHostEqual = TestCredentials.DefaultHostOverride == context.TargetHost; + isPemEqual = certChain == context.TargetPem; + return returnValue; + } + private class SslCredentialsTestServiceImpl : TestService.TestServiceBase { public override Task UnaryCall(SimpleRequest request, ServerCallContext context) diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index dc690a6a608..5f80d3417a5 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -927,20 +927,53 @@ grpcsharp_override_default_ssl_roots(const char* pem_root_certs) { grpc_set_ssl_roots_override_callback(override_ssl_roots_handler); } +typedef int(GPR_CALLTYPE* grpcsharp_verify_peer_func)(const char* target_host, + const char* target_pem, + void* userdata, + int32_t isDestroy); + +static void grpcsharp_verify_peer_destroy_handler(void* userdata) { + grpcsharp_verify_peer_func callback = + (grpcsharp_verify_peer_func)(intptr_t)userdata; + callback(NULL, NULL, NULL, 1); +} + +static int grpcsharp_verify_peer_handler(const char* target_host, + const char* target_pem, + void* userdata) { + grpcsharp_verify_peer_func callback = + (grpcsharp_verify_peer_func)(intptr_t)userdata; + return callback(target_host, target_pem, NULL, 0); +} + + GPR_EXPORT grpc_channel_credentials* GPR_CALLTYPE grpcsharp_ssl_credentials_create(const char* pem_root_certs, const char* key_cert_pair_cert_chain, - const char* key_cert_pair_private_key) { + const char* key_cert_pair_private_key, + grpcsharp_verify_peer_func verify_peer_func) { grpc_ssl_pem_key_cert_pair key_cert_pair; + verify_peer_options verify_options; + verify_peer_options* p_verify_options = NULL; + if (verify_peer_func != NULL) { + verify_options.verify_peer_callback_userdata = + (void*)(intptr_t)verify_peer_func; + verify_options.verify_peer_destruct = + grpcsharp_verify_peer_destroy_handler; + verify_options.verify_peer_callback = grpcsharp_verify_peer_handler; + p_verify_options = &verify_options; + } + if (key_cert_pair_cert_chain || key_cert_pair_private_key) { key_cert_pair.cert_chain = key_cert_pair_cert_chain; key_cert_pair.private_key = key_cert_pair_private_key; - return grpc_ssl_credentials_create(pem_root_certs, &key_cert_pair, NULL, - NULL); + return grpc_ssl_credentials_create(pem_root_certs, &key_cert_pair, + p_verify_options, NULL); } else { GPR_ASSERT(!key_cert_pair_cert_chain); GPR_ASSERT(!key_cert_pair_private_key); - return grpc_ssl_credentials_create(pem_root_certs, NULL, NULL, NULL); + return grpc_ssl_credentials_create(pem_root_certs, NULL, p_verify_options, + NULL); } } diff --git a/templates/src/csharp/Grpc.Core/Internal/native_methods.include b/templates/src/csharp/Grpc.Core/Internal/native_methods.include index b7a8e285488..4cb71730529 100644 --- a/templates/src/csharp/Grpc.Core/Internal/native_methods.include +++ b/templates/src/csharp/Grpc.Core/Internal/native_methods.include @@ -44,7 +44,7 @@ native_method_signatures = [ 'void grpcsharp_channel_args_set_integer(ChannelArgsSafeHandle args, UIntPtr index, string key, int value)', 'void grpcsharp_channel_args_destroy(IntPtr args)', 'void grpcsharp_override_default_ssl_roots(string pemRootCerts)', - 'ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey)', + 'ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback)', 'ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds)', 'void grpcsharp_channel_credentials_release(IntPtr credentials)', 'ChannelSafeHandle grpcsharp_insecure_channel_create(string target, ChannelArgsSafeHandle channelArgs)', From f26ef91c98c6f3c917b3eaec0f388b2c13c85c77 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 22 Mar 2019 08:26:10 -0700 Subject: [PATCH 06/30] Convert external connectivity watcher to C++. --- .../filters/client_channel/client_channel.cc | 320 ++++++++++-------- 1 file changed, 180 insertions(+), 140 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 891df3baf6b..b6a9e83d33b 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -50,6 +50,7 @@ #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/inlined_vector.h" #include "src/core/lib/gprpp/manual_constructor.h" +#include "src/core/lib/gprpp/mutex_lock.h" #include "src/core/lib/iomgr/combiner.h" #include "src/core/lib/iomgr/iomgr.h" #include "src/core/lib/iomgr/polling_entity.h" @@ -91,7 +92,53 @@ grpc_core::TraceFlag grpc_client_channel_routing_trace( * CHANNEL-WIDE FUNCTIONS */ -struct external_connectivity_watcher; +// Forward declaration. +typedef struct client_channel_channel_data channel_data; + +namespace grpc_core { +namespace { + +class ExternalConnectivityWatcher { + public: + class WatcherList { + public: + WatcherList() { gpr_mu_init(&mu_); } + ~WatcherList() { gpr_mu_destroy(&mu_); } + + int size() const; + ExternalConnectivityWatcher* Lookup(grpc_closure* on_complete) const; + void Append(ExternalConnectivityWatcher* watcher); + void Remove(ExternalConnectivityWatcher* watcher); + + private: + // head_ is guarded by its own mutex, since the size of the list needs + // to be grabbed immediately without polling on a CQ. + mutable gpr_mu mu_; + ExternalConnectivityWatcher* head_ = nullptr; + }; + + ExternalConnectivityWatcher(channel_data* chand, grpc_polling_entity pollent, + grpc_connectivity_state* state, + grpc_closure* on_complete, + grpc_closure* watcher_timer_init); + + ~ExternalConnectivityWatcher(); + + private: + static void OnExternalWatchCompleteLocked(void* arg, grpc_error* error); + static void WatchConnectivityStateLocked(void* arg, grpc_error* ignored); + + channel_data* chand_; + grpc_polling_entity pollent_; + grpc_connectivity_state* state_; + grpc_closure* on_complete_; + grpc_closure* watcher_timer_init_; + grpc_closure my_closure_; + ExternalConnectivityWatcher* next_ = nullptr; +}; + +} // namespace +} // namespace grpc_core struct QueuedPick { LoadBalancingPolicy::PickArgs pick; @@ -99,7 +146,7 @@ struct QueuedPick { QueuedPick* next = nullptr; }; -typedef struct client_channel_channel_data { +struct client_channel_channel_data { bool deadline_checking_enabled; bool enable_retries; size_t per_rpc_retry_buffer_size; @@ -139,11 +186,10 @@ typedef struct client_channel_channel_data { grpc_connectivity_state_tracker state_tracker; grpc_error* disconnect_error; - /* external_connectivity_watcher_list head is guarded by its own mutex, since - * counts need to be grabbed immediately without polling on a cq */ - gpr_mu external_connectivity_watcher_list_mu; - struct external_connectivity_watcher* external_connectivity_watcher_list_head; -} channel_data; + grpc_core::ManualConstructor< + grpc_core::ExternalConnectivityWatcher::WatcherList> + external_connectivity_watcher_list; +}; // Forward declarations. static void start_pick_locked(void* arg, grpc_error* ignored); @@ -191,6 +237,124 @@ static void set_connectivity_state_and_picker_locked( namespace grpc_core { namespace { +// +// ExternalConnectivityWatcher::WatcherList +// + +int ExternalConnectivityWatcher::WatcherList::size() const { + MutexLock lock(&mu_); + int count = 0; + for (ExternalConnectivityWatcher* w = head_; w != nullptr; w = w->next_) { + ++count; + } + return count; +} + +ExternalConnectivityWatcher* ExternalConnectivityWatcher::WatcherList::Lookup( + grpc_closure* on_complete) const { + MutexLock lock(&mu_); + ExternalConnectivityWatcher* w = head_; + while (w != nullptr && w->on_complete_ != on_complete) { + w = w->next_; + } + return w; +} + +void ExternalConnectivityWatcher::WatcherList::Append( + ExternalConnectivityWatcher* watcher) { + GPR_ASSERT(!Lookup(watcher->on_complete_)); + MutexLock lock(&mu_); + GPR_ASSERT(watcher->next_ == nullptr); + watcher->next_ = head_; + head_ = watcher; +} + +void ExternalConnectivityWatcher::WatcherList::Remove( + ExternalConnectivityWatcher* watcher) { + MutexLock lock(&mu_); + if (watcher == head_) { + head_ = watcher->next_; + return; + } + for (ExternalConnectivityWatcher* w = head_; w != nullptr; w = w->next_) { + if (w->next_ == watcher) { + w->next_ = w->next_->next_; + return; + } + } + GPR_UNREACHABLE_CODE(return ); +} + +// +// ExternalConnectivityWatcher +// + +ExternalConnectivityWatcher::ExternalConnectivityWatcher( + channel_data* chand, grpc_polling_entity pollent, + grpc_connectivity_state* state, grpc_closure* on_complete, + grpc_closure* watcher_timer_init) + : chand_(chand), + pollent_(pollent), + state_(state), + on_complete_(on_complete), + watcher_timer_init_(watcher_timer_init) { + grpc_polling_entity_add_to_pollset_set(&pollent_, chand_->interested_parties); + GRPC_CHANNEL_STACK_REF(chand_->owning_stack, "ExternalConnectivityWatcher"); + GRPC_CLOSURE_SCHED( + GRPC_CLOSURE_INIT(&my_closure_, WatchConnectivityStateLocked, this, + grpc_combiner_scheduler(chand_->combiner)), + GRPC_ERROR_NONE); +} + +ExternalConnectivityWatcher::~ExternalConnectivityWatcher() { + grpc_polling_entity_del_from_pollset_set(&pollent_, + chand_->interested_parties); + GRPC_CHANNEL_STACK_UNREF(chand_->owning_stack, "ExternalConnectivityWatcher"); +} + +void ExternalConnectivityWatcher::OnExternalWatchCompleteLocked( + void* arg, grpc_error* error) { + ExternalConnectivityWatcher* self = + static_cast(arg); + grpc_closure* on_complete = self->on_complete_; + self->chand_->external_connectivity_watcher_list->Remove(self); + Delete(self); + GRPC_CLOSURE_SCHED(on_complete, GRPC_ERROR_REF(error)); +} + +void ExternalConnectivityWatcher::WatchConnectivityStateLocked( + void* arg, grpc_error* ignored) { + ExternalConnectivityWatcher* self = + static_cast(arg); + if (self->state_ == nullptr) { + // Handle cancellation. + GPR_ASSERT(self->watcher_timer_init_ == nullptr); + ExternalConnectivityWatcher* found = + self->chand_->external_connectivity_watcher_list->Lookup( + self->on_complete_); + if (found != nullptr) { + GPR_ASSERT(found->on_complete_ == self->on_complete_); + grpc_connectivity_state_notify_on_state_change( + &found->chand_->state_tracker, nullptr, &found->my_closure_); + } + Delete(self); + return; + } + // New watcher. + self->chand_->external_connectivity_watcher_list->Append(self); + // This assumes that the closure is scheduled on the ExecCtx scheduler + // and that GRPC_CLOSURE_RUN would run the closure immediately. + GRPC_CLOSURE_RUN(self->watcher_timer_init_, GRPC_ERROR_NONE); + GRPC_CLOSURE_INIT(&self->my_closure_, OnExternalWatchCompleteLocked, self, + grpc_combiner_scheduler(self->chand_->combiner)); + grpc_connectivity_state_notify_on_state_change( + &self->chand_->state_tracker, self->state_, &self->my_closure_); +} + +// +// ClientChannelControlHelper +// + class ClientChannelControlHelper : public LoadBalancingPolicy::ChannelControlHelper { public: @@ -402,12 +566,7 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem, "client_channel"); chand->disconnect_error = GRPC_ERROR_NONE; gpr_mu_init(&chand->info_mu); - gpr_mu_init(&chand->external_connectivity_watcher_list_mu); - - gpr_mu_lock(&chand->external_connectivity_watcher_list_mu); - chand->external_connectivity_watcher_list_head = nullptr; - gpr_mu_unlock(&chand->external_connectivity_watcher_list_mu); - + chand->external_connectivity_watcher_list.Init(); chand->owning_stack = args->channel_stack; chand->deadline_checking_enabled = grpc_deadline_checking_enabled(args->channel_args); @@ -515,7 +674,7 @@ static void cc_destroy_channel_elem(grpc_channel_element* elem) { GRPC_ERROR_UNREF(chand->disconnect_error); grpc_connectivity_state_destroy(&chand->state_tracker); gpr_mu_destroy(&chand->info_mu); - gpr_mu_destroy(&chand->external_connectivity_watcher_list_mu); + chand->external_connectivity_watcher_list.Destroy(); } /************************************************************************* @@ -2875,6 +3034,10 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; +// +// functions exported to the rest of core +// + void grpc_client_channel_set_channelz_node( grpc_channel_element* elem, grpc_core::channelz::ClientChannelNode* node) { channel_data* chand = static_cast(elem->channel_data); @@ -2914,120 +3077,10 @@ grpc_connectivity_state grpc_client_channel_check_connectivity_state( return out; } -typedef struct external_connectivity_watcher { - channel_data* chand; - grpc_polling_entity pollent; - grpc_closure* on_complete; - grpc_closure* watcher_timer_init; - grpc_connectivity_state* state; - grpc_closure my_closure; - struct external_connectivity_watcher* next; -} external_connectivity_watcher; - -static external_connectivity_watcher* lookup_external_connectivity_watcher( - channel_data* chand, grpc_closure* on_complete) { - gpr_mu_lock(&chand->external_connectivity_watcher_list_mu); - external_connectivity_watcher* w = - chand->external_connectivity_watcher_list_head; - while (w != nullptr && w->on_complete != on_complete) { - w = w->next; - } - gpr_mu_unlock(&chand->external_connectivity_watcher_list_mu); - return w; -} - -static void external_connectivity_watcher_list_append( - channel_data* chand, external_connectivity_watcher* w) { - GPR_ASSERT(!lookup_external_connectivity_watcher(chand, w->on_complete)); - - gpr_mu_lock(&w->chand->external_connectivity_watcher_list_mu); - GPR_ASSERT(!w->next); - w->next = chand->external_connectivity_watcher_list_head; - chand->external_connectivity_watcher_list_head = w; - gpr_mu_unlock(&w->chand->external_connectivity_watcher_list_mu); -} - -static void external_connectivity_watcher_list_remove( - channel_data* chand, external_connectivity_watcher* to_remove) { - GPR_ASSERT( - lookup_external_connectivity_watcher(chand, to_remove->on_complete)); - gpr_mu_lock(&chand->external_connectivity_watcher_list_mu); - if (to_remove == chand->external_connectivity_watcher_list_head) { - chand->external_connectivity_watcher_list_head = to_remove->next; - gpr_mu_unlock(&chand->external_connectivity_watcher_list_mu); - return; - } - external_connectivity_watcher* w = - chand->external_connectivity_watcher_list_head; - while (w != nullptr) { - if (w->next == to_remove) { - w->next = w->next->next; - gpr_mu_unlock(&chand->external_connectivity_watcher_list_mu); - return; - } - w = w->next; - } - GPR_UNREACHABLE_CODE(return ); -} - int grpc_client_channel_num_external_connectivity_watchers( grpc_channel_element* elem) { channel_data* chand = static_cast(elem->channel_data); - int count = 0; - - gpr_mu_lock(&chand->external_connectivity_watcher_list_mu); - external_connectivity_watcher* w = - chand->external_connectivity_watcher_list_head; - while (w != nullptr) { - count++; - w = w->next; - } - gpr_mu_unlock(&chand->external_connectivity_watcher_list_mu); - - return count; -} - -static void on_external_watch_complete_locked(void* arg, grpc_error* error) { - external_connectivity_watcher* w = - static_cast(arg); - grpc_closure* follow_up = w->on_complete; - grpc_polling_entity_del_from_pollset_set(&w->pollent, - w->chand->interested_parties); - GRPC_CHANNEL_STACK_UNREF(w->chand->owning_stack, - "external_connectivity_watcher"); - external_connectivity_watcher_list_remove(w->chand, w); - gpr_free(w); - GRPC_CLOSURE_SCHED(follow_up, GRPC_ERROR_REF(error)); -} - -static void watch_connectivity_state_locked(void* arg, - grpc_error* error_ignored) { - external_connectivity_watcher* w = - static_cast(arg); - external_connectivity_watcher* found = nullptr; - if (w->state != nullptr) { - external_connectivity_watcher_list_append(w->chand, w); - // An assumption is being made that the closure is scheduled on the exec ctx - // scheduler and that GRPC_CLOSURE_RUN would run the closure immediately. - GRPC_CLOSURE_RUN(w->watcher_timer_init, GRPC_ERROR_NONE); - GRPC_CLOSURE_INIT(&w->my_closure, on_external_watch_complete_locked, w, - grpc_combiner_scheduler(w->chand->combiner)); - grpc_connectivity_state_notify_on_state_change(&w->chand->state_tracker, - w->state, &w->my_closure); - } else { - GPR_ASSERT(w->watcher_timer_init == nullptr); - found = lookup_external_connectivity_watcher(w->chand, w->on_complete); - if (found) { - GPR_ASSERT(found->on_complete == w->on_complete); - grpc_connectivity_state_notify_on_state_change( - &found->chand->state_tracker, nullptr, &found->my_closure); - } - grpc_polling_entity_del_from_pollset_set(&w->pollent, - w->chand->interested_parties); - GRPC_CHANNEL_STACK_UNREF(w->chand->owning_stack, - "external_connectivity_watcher"); - gpr_free(w); - } + return chand->external_connectivity_watcher_list->size(); } void grpc_client_channel_watch_connectivity_state( @@ -3035,21 +3088,8 @@ void grpc_client_channel_watch_connectivity_state( grpc_connectivity_state* state, grpc_closure* closure, grpc_closure* watcher_timer_init) { channel_data* chand = static_cast(elem->channel_data); - external_connectivity_watcher* w = - static_cast(gpr_zalloc(sizeof(*w))); - w->chand = chand; - w->pollent = pollent; - w->on_complete = closure; - w->state = state; - w->watcher_timer_init = watcher_timer_init; - grpc_polling_entity_add_to_pollset_set(&w->pollent, - chand->interested_parties); - GRPC_CHANNEL_STACK_REF(w->chand->owning_stack, - "external_connectivity_watcher"); - GRPC_CLOSURE_SCHED( - GRPC_CLOSURE_INIT(&w->my_closure, watch_connectivity_state_locked, w, - grpc_combiner_scheduler(chand->combiner)), - GRPC_ERROR_NONE); + grpc_core::New( + chand, pollent, state, closure, watcher_timer_init); } grpc_core::RefCountedPtr From 9ff845961ea95c6beb4236fac3770fcc21b768b0 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Tue, 26 Mar 2019 19:21:18 +0100 Subject: [PATCH 07/30] Breakout of #18445 - part 2 --- src/core/lib/transport/transport.h | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h index 9d4b5615e57..8631a1af81f 100644 --- a/src/core/lib/transport/transport.h +++ b/src/core/lib/transport/transport.h @@ -273,40 +273,40 @@ struct grpc_transport_stream_op_batch_payload { /** Transport op: a set of operations to perform on a transport as a whole */ typedef struct grpc_transport_op { /** Called when processing of this op is done. */ - grpc_closure* on_consumed; + grpc_closure* on_consumed = nullptr; /** connectivity monitoring - set connectivity_state to NULL to unsubscribe */ - grpc_closure* on_connectivity_state_change; - grpc_connectivity_state* connectivity_state; + grpc_closure* on_connectivity_state_change = nullptr; + grpc_connectivity_state* connectivity_state = nullptr; /** should the transport be disconnected * Error contract: the transport that gets this op must cause * disconnect_with_error to be unref'ed after processing it */ - grpc_error* disconnect_with_error; + grpc_error* disconnect_with_error = nullptr; /** what should the goaway contain? * Error contract: the transport that gets this op must cause * goaway_error to be unref'ed after processing it */ - grpc_error* goaway_error; + grpc_error* goaway_error = nullptr; /** set the callback for accepting new streams; this is a permanent callback, unlike the other one-shot closures. If true, the callback is set to set_accept_stream_fn, with its user_data argument set to set_accept_stream_user_data */ - bool set_accept_stream; + bool set_accept_stream = false; void (*set_accept_stream_fn)(void* user_data, grpc_transport* transport, - const void* server_data); - void* set_accept_stream_user_data; + const void* server_data) = nullptr; + void* set_accept_stream_user_data = nullptr; /** add this transport to a pollset */ - grpc_pollset* bind_pollset; + grpc_pollset* bind_pollset = nullptr; /** add this transport to a pollset_set */ - grpc_pollset_set* bind_pollset_set; + grpc_pollset_set* bind_pollset_set = nullptr; /** send a ping, if either on_initiate or on_ack is not NULL */ struct { /** Ping may be delayed by the transport, on_initiate callback will be called when the ping is actually being sent. */ - grpc_closure* on_initiate; + grpc_closure* on_initiate = nullptr; /** Called when the ping ack is received */ - grpc_closure* on_ack; + grpc_closure* on_ack = nullptr; } send_ping; // If true, will reset the channel's connection backoff. - bool reset_connect_backoff; + bool reset_connect_backoff = false; /*************************************************************************** * remaining fields are initialized and used at the discretion of the From faa4bb8b0c077bd29b2dd4afdd4500de62f7d29b Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 27 Mar 2019 15:35:37 -0700 Subject: [PATCH 08/30] Allow user to explicitly disable CFStream with env var "grpc_cfstream=0" --- src/objective-c/GRPCClient/GRPCCall.m | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 563adeca0d1..679b505ec33 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -434,7 +434,9 @@ const char *kCFStreamVarName = "grpc_cfstream"; // Guarantees the code in {} block is invoked only once. See ref at: // https://developer.apple.com/documentation/objectivec/nsobject/1418639-initialize?language=objc if (self == [GRPCCall self]) { - setenv(kCFStreamVarName, "1", 1); + // Enable CFStream by default by do not overwrite if the user explicitly disables CFStream with + // environment variable "grpc_cfstream=0" + setenv(kCFStreamVarName, "1", 0); grpc_init(); callFlags = [NSMutableDictionary dictionary]; } From a1fe33a0d4f13bfbab3e44afc6e5329a7a100b1a Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 28 Mar 2019 15:03:55 -0700 Subject: [PATCH 09/30] Remove grpc_cfstream from BUILD --- BUILD | 2 -- 1 file changed, 2 deletions(-) diff --git a/BUILD b/BUILD index b9fba4bef85..c56107d102e 100644 --- a/BUILD +++ b/BUILD @@ -307,7 +307,6 @@ grpc_cc_library( public_hdrs = GRPC_PUBLIC_HDRS + GRPC_SECURE_PUBLIC_HDRS, standalone = True, deps = [ - "grpc_cfstream", "grpc_common", "grpc_lb_policy_grpclb_secure", "grpc_lb_policy_xds_secure", @@ -361,7 +360,6 @@ grpc_cc_library( "gpr", "grpc", "grpc++_base", - "grpc_cfstream", "grpc++_codegen_base", "grpc++_codegen_base_src", "grpc++_codegen_proto", From 62e4234470fb721e3ed8ef454092378f80b84745 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 28 Mar 2019 14:21:00 -0700 Subject: [PATCH 10/30] Add API deprecation notice to generated code --- src/compiler/objective_c_generator.cc | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/compiler/objective_c_generator.cc b/src/compiler/objective_c_generator.cc index e806f46d814..960fd808dd8 100644 --- a/src/compiler/objective_c_generator.cc +++ b/src/compiler/objective_c_generator.cc @@ -70,6 +70,10 @@ static void PrintAllComments(const DescriptorType* desc, Printer* printer) { } printer->Print("\n"); } + printer->Print(" *\n"); + printer->Print( + " * This method belongs to a set of APIs that have been deprecated. Using" + " the v2 API is recommended.\n"); printer->Print(" */\n"); } @@ -278,6 +282,13 @@ void PrintMethodImplementations(Printer* printer, map< ::grpc::string, ::grpc::string> vars = { {"service_class", ServiceClassName(service)}}; + printer.Print(vars, + "/**\n" + " * The methods in this protocol belong to a set of old APIs " + "that have been deprecated. They do not\n" + " * recognize call options provided in the initializer. Using " + "the v2 protocol is recommended.\n" + " */\n"); printer.Print(vars, "@protocol $service_class$ \n\n"); for (int i = 0; i < service->method_count(); i++) { PrintMethodDeclarations(&printer, service->method(i)); @@ -329,10 +340,13 @@ void PrintMethodImplementations(Printer* printer, "callOptions:(GRPCCallOptions " "*_Nullable)callOptions" " NS_DESIGNATED_INITIALIZER;\n"); - printer.Print("- (instancetype)initWithHost:(NSString *)host;\n"); printer.Print( "+ (instancetype)serviceWithHost:(NSString *)host " "callOptions:(GRPCCallOptions *_Nullable)callOptions;\n"); + printer.Print( + "// The following methods belong to a set of old APIs that have been " + "deprecated.\n"); + printer.Print("- (instancetype)initWithHost:(NSString *)host;\n"); printer.Print("+ (instancetype)serviceWithHost:(NSString *)host;\n"); printer.Print("@end\n"); From b400a6bf2014583bc6ee789c2b0f0ac031693d43 Mon Sep 17 00:00:00 2001 From: Nicolas Noble Date: Fri, 29 Mar 2019 10:29:45 -0700 Subject: [PATCH 11/30] Revert "Fix compilation error in JWT using const_cast." --- src/core/lib/security/credentials/jwt/jwt_verifier.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/lib/security/credentials/jwt/jwt_verifier.cc b/src/core/lib/security/credentials/jwt/jwt_verifier.cc index d573c30787d..5b120eddb43 100644 --- a/src/core/lib/security/credentials/jwt/jwt_verifier.cc +++ b/src/core/lib/security/credentials/jwt/jwt_verifier.cc @@ -624,9 +624,8 @@ static int verify_jwt_signature(EVP_PKEY* key, const char* alg, gpr_log(GPR_ERROR, "EVP_DigestVerifyUpdate failed."); goto end; } - if (EVP_DigestVerifyFinal( - md_ctx, const_cast(GRPC_SLICE_START_PTR(signature)), - GRPC_SLICE_LENGTH(signature)) != 1) { + if (EVP_DigestVerifyFinal(md_ctx, GRPC_SLICE_START_PTR(signature), + GRPC_SLICE_LENGTH(signature)) != 1) { gpr_log(GPR_ERROR, "JWT signature verification failed."); goto end; } From f1199912e85e8cb7cc03c3b2c1751797cd474316 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Sat, 30 Mar 2019 22:30:18 -0700 Subject: [PATCH 12/30] fix typo in log message --- src/csharp/Grpc.Core/Internal/NativeCallbackDispatcher.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/csharp/Grpc.Core/Internal/NativeCallbackDispatcher.cs b/src/csharp/Grpc.Core/Internal/NativeCallbackDispatcher.cs index d5146e816e2..0777d7933e6 100644 --- a/src/csharp/Grpc.Core/Internal/NativeCallbackDispatcher.cs +++ b/src/csharp/Grpc.Core/Internal/NativeCallbackDispatcher.cs @@ -63,7 +63,7 @@ namespace Grpc.Core.Internal catch (Exception e) { // eat the exception, we must not throw when inside callback from native code. - Logger.Error(e, "Caught exception inside callback from native callback."); + Logger.Error(e, "Caught exception inside callback from native code."); return 0; } } From c835fedd4d42d7799a9a0c89b284db9c9a66a886 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Thu, 21 Mar 2019 12:07:08 -0700 Subject: [PATCH 13/30] Modify verifyPeerCallback logic to use NativeCallbackDispatcher --- src/csharp/Grpc.Core/ChannelCredentials.cs | 61 +++++++----- .../Internal/ChannelCredentialsSafeHandle.cs | 12 +-- .../Internal/NativeMethods.Generated.cs | 6 +- .../SslCredentialsTest.cs | 92 ++++++++++--------- src/csharp/ext/grpc_csharp_ext.c | 53 +++++------ .../Grpc.Core/Internal/native_methods.include | 2 +- 6 files changed, 121 insertions(+), 105 deletions(-) diff --git a/src/csharp/Grpc.Core/ChannelCredentials.cs b/src/csharp/Grpc.Core/ChannelCredentials.cs index 103a594bb06..2ecd3875e84 100644 --- a/src/csharp/Grpc.Core/ChannelCredentials.cs +++ b/src/csharp/Grpc.Core/ChannelCredentials.cs @@ -127,8 +127,6 @@ namespace Grpc.Core readonly string rootCertificates; readonly KeyCertificatePair keyCertificatePair; readonly VerifyPeerCallback verifyPeerCallback; - readonly VerifyPeerCallbackInternal verifyPeerCallbackInternal; - readonly GCHandle gcHandle; /// /// Creates client-side SSL credentials loaded from @@ -168,12 +166,7 @@ namespace Grpc.Core { this.rootCertificates = rootCertificates; this.keyCertificatePair = keyCertificatePair; - if (verifyPeerCallback != null) - { - this.verifyPeerCallback = verifyPeerCallback; - this.verifyPeerCallbackInternal = this.VerifyPeerCallbackHandler; - gcHandle = GCHandle.Alloc(verifyPeerCallbackInternal); - } + this.verifyPeerCallback = verifyPeerCallback; } /// @@ -207,29 +200,53 @@ namespace Grpc.Core internal override ChannelCredentialsSafeHandle CreateNativeCredentials() { - return ChannelCredentialsSafeHandle.CreateSslCredentials(rootCertificates, keyCertificatePair, this.verifyPeerCallbackInternal); + IntPtr verifyPeerCallbackTag = IntPtr.Zero; + if (verifyPeerCallback != null) + { + verifyPeerCallbackTag = new VerifyPeerCallbackRegistration(verifyPeerCallback).CallbackRegistration.Tag; + } + return ChannelCredentialsSafeHandle.CreateSslCredentials(rootCertificates, keyCertificatePair, verifyPeerCallbackTag); } - private int VerifyPeerCallbackHandler(IntPtr host, IntPtr pem, IntPtr userData, bool isDestroy) + private class VerifyPeerCallbackRegistration { - if (isDestroy) + readonly VerifyPeerCallback verifyPeerCallback; + readonly NativeCallbackRegistration callbackRegistration; + + public VerifyPeerCallbackRegistration(VerifyPeerCallback verifyPeerCallback) { - this.gcHandle.Free(); - return 0; + this.verifyPeerCallback = verifyPeerCallback; + this.callbackRegistration = NativeCallbackDispatcher.RegisterCallback(HandleUniversalCallback); } - try - { - var context = new VerifyPeerContext(Marshal.PtrToStringAnsi(host), Marshal.PtrToStringAnsi(pem)); + public NativeCallbackRegistration CallbackRegistration => callbackRegistration; - return this.verifyPeerCallback(context) ? 0 : 1; + private int HandleUniversalCallback(IntPtr arg0, IntPtr arg1, IntPtr arg2, IntPtr arg3, IntPtr arg4, IntPtr arg5) + { + return VerifyPeerCallbackHandler(arg0, arg1, arg2 != IntPtr.Zero); } - catch (Exception e) + + private int VerifyPeerCallbackHandler(IntPtr host, IntPtr pem, bool isDestroy) { - // eat the exception, we must not throw when inside callback from native code. - Logger.Error(e, "Exception occurred while invoking verify peer callback handler."); - // Return validation failure in case of exception. - return 1; + if (isDestroy) + { + this.callbackRegistration.Dispose(); + return 0; + } + + try + { + var context = new VerifyPeerContext(Marshal.PtrToStringAnsi(host), Marshal.PtrToStringAnsi(pem)); + + return this.verifyPeerCallback(context) ? 0 : 1; + } + catch (Exception e) + { + // eat the exception, we must not throw when inside callback from native code. + Logger.Error(e, "Exception occurred while invoking verify peer callback handler."); + // Return validation failure in case of exception. + return 1; + } } } } diff --git a/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs b/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs index 5e336673153..d4f50344b23 100644 --- a/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/ChannelCredentialsSafeHandle.cs @@ -20,12 +20,6 @@ using System.Threading.Tasks; namespace Grpc.Core.Internal { - internal delegate int VerifyPeerCallbackInternal( - IntPtr targetHost, - IntPtr targetPem, - IntPtr userData, - bool isDestroy); - /// /// grpc_channel_credentials from grpc/grpc_security.h /// @@ -44,15 +38,15 @@ namespace Grpc.Core.Internal return creds; } - public static ChannelCredentialsSafeHandle CreateSslCredentials(string pemRootCerts, KeyCertificatePair keyCertPair, VerifyPeerCallbackInternal verifyPeerCallback) + public static ChannelCredentialsSafeHandle CreateSslCredentials(string pemRootCerts, KeyCertificatePair keyCertPair, IntPtr verifyPeerCallbackTag) { if (keyCertPair != null) { - return Native.grpcsharp_ssl_credentials_create(pemRootCerts, keyCertPair.CertificateChain, keyCertPair.PrivateKey, verifyPeerCallback); + return Native.grpcsharp_ssl_credentials_create(pemRootCerts, keyCertPair.CertificateChain, keyCertPair.PrivateKey, verifyPeerCallbackTag); } else { - return Native.grpcsharp_ssl_credentials_create(pemRootCerts, null, null, verifyPeerCallback); + return Native.grpcsharp_ssl_credentials_create(pemRootCerts, null, null, verifyPeerCallbackTag); } } diff --git a/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs b/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs index e09b9813f79..a1387aff562 100644 --- a/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs +++ b/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs @@ -482,7 +482,7 @@ namespace Grpc.Core.Internal public delegate void grpcsharp_channel_args_set_integer_delegate(ChannelArgsSafeHandle args, UIntPtr index, string key, int value); public delegate void grpcsharp_channel_args_destroy_delegate(IntPtr args); public delegate void grpcsharp_override_default_ssl_roots_delegate(string pemRootCerts); - public delegate ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create_delegate(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback); + public delegate ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create_delegate(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, IntPtr verifyPeerCallbackTag); public delegate ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create_delegate(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds); public delegate void grpcsharp_channel_credentials_release_delegate(IntPtr credentials); public delegate ChannelSafeHandle grpcsharp_insecure_channel_create_delegate(string target, ChannelArgsSafeHandle channelArgs); @@ -676,7 +676,7 @@ namespace Grpc.Core.Internal public static extern void grpcsharp_override_default_ssl_roots(string pemRootCerts); [DllImport(ImportName)] - public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback); + public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, IntPtr verifyPeerCallbackTag); [DllImport(ImportName)] public static extern ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds); @@ -972,7 +972,7 @@ namespace Grpc.Core.Internal public static extern void grpcsharp_override_default_ssl_roots(string pemRootCerts); [DllImport(ImportName)] - public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback); + public static extern ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, IntPtr verifyPeerCallbackTag); [DllImport(ImportName)] public static extern ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds); diff --git a/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs b/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs index 818ac6d1adc..4c1189320fa 100644 --- a/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs +++ b/src/csharp/Grpc.IntegrationTesting/SslCredentialsTest.cs @@ -44,23 +44,18 @@ namespace Grpc.IntegrationTesting string rootCert; KeyCertificatePair keyCertPair; - string certChain; - List options; - bool isHostEqual; - bool isPemEqual; public void InitClientAndServer(bool clientAddKeyCertPair, - SslClientCertificateRequestType clientCertRequestType) + SslClientCertificateRequestType clientCertRequestType, + VerifyPeerCallback verifyPeerCallback = null) { rootCert = File.ReadAllText(TestCredentials.ClientCertAuthorityPath); - certChain = File.ReadAllText(TestCredentials.ServerCertChainPath); - certChain = certChain.Replace("\r", string.Empty); keyCertPair = new KeyCertificatePair( - certChain, + File.ReadAllText(TestCredentials.ServerCertChainPath), File.ReadAllText(TestCredentials.ServerPrivateKeyPath)); var serverCredentials = new SslServerCredentials(new[] { keyCertPair }, rootCert, clientCertRequestType); - var clientCredentials = clientAddKeyCertPair ? new SslCredentials(rootCert, keyCertPair, context => this.VerifyPeerCallback(context, true)) : new SslCredentials(rootCert); + var clientCredentials = new SslCredentials(rootCert, clientAddKeyCertPair ? keyCertPair : null, verifyPeerCallback); // Disable SO_REUSEPORT to prevent https://github.com/grpc/grpc/issues/10755 server = new Server(new[] { new ChannelOption(ChannelOptions.SoReuseport, 0) }) @@ -70,7 +65,7 @@ namespace Grpc.IntegrationTesting }; server.Start(); - options = new List + var options = new List { new ChannelOption(ChannelOptions.SslTargetNameOverride, TestCredentials.DefaultHostOverride) }; @@ -194,6 +189,52 @@ namespace Grpc.IntegrationTesting Assert.Throws(typeof(ArgumentNullException), () => new SslServerCredentials(keyCertPairs, null, SslClientCertificateRequestType.RequestAndRequireAndVerify)); } + [Test] + public async Task VerifyPeerCallback_Accepted() + { + string targetNameFromCallback = null; + string peerPemFromCallback = null; + InitClientAndServer( + clientAddKeyCertPair: false, + clientCertRequestType: SslClientCertificateRequestType.DontRequest, + verifyPeerCallback: (ctx) => + { + targetNameFromCallback = ctx.TargetName; + peerPemFromCallback = ctx.PeerPem; + return true; + }); + await CheckAccepted(expectPeerAuthenticated: false); + Assert.AreEqual(TestCredentials.DefaultHostOverride, targetNameFromCallback); + var expectedServerPem = File.ReadAllText(TestCredentials.ServerCertChainPath).Replace("\r", ""); + Assert.AreEqual(expectedServerPem, peerPemFromCallback); + } + + [Test] + public void VerifyPeerCallback_CallbackThrows_Rejected() + { + InitClientAndServer( + clientAddKeyCertPair: false, + clientCertRequestType: SslClientCertificateRequestType.DontRequest, + verifyPeerCallback: (ctx) => + { + throw new Exception("VerifyPeerCallback has thrown on purpose."); + }); + CheckRejected(); + } + + [Test] + public void VerifyPeerCallback_Rejected() + { + InitClientAndServer( + clientAddKeyCertPair: false, + clientCertRequestType: SslClientCertificateRequestType.DontRequest, + verifyPeerCallback: (ctx) => + { + return false; + }); + CheckRejected(); + } + private async Task CheckAccepted(bool expectPeerAuthenticated) { var call = client.UnaryCallAsync(new SimpleRequest { ResponseSize = 10 }); @@ -216,37 +257,6 @@ namespace Grpc.IntegrationTesting Assert.AreEqual(12345, response.AggregatedPayloadSize); } - [Test] - public void VerifyPeerCallbackTest() - { - InitClientAndServer(true, SslClientCertificateRequestType.RequestAndRequireAndVerify); - - // Force GC collection to verify that the VerifyPeerCallback is not collected. If - // it gets collected, this test will hang. - GC.Collect(); - - client.UnaryCall(new SimpleRequest { ResponseSize = 10 }); - Assert.IsTrue(isHostEqual); - Assert.IsTrue(isPemEqual); - } - - [Test] - public void VerifyPeerCallbackFailTest() - { - InitClientAndServer(true, SslClientCertificateRequestType.RequestAndRequireAndVerify); - var clientCredentials = new SslCredentials(rootCert, keyCertPair, context => this.VerifyPeerCallback(context, false)); - var failingChannel = new Channel(Host, server.Ports.Single().BoundPort, clientCredentials, options); - var failingClient = new TestService.TestServiceClient(failingChannel); - Assert.Throws(() => failingClient.UnaryCall(new SimpleRequest { ResponseSize = 10 })); - } - - private bool VerifyPeerCallback(VerifyPeerContext context, bool returnValue) - { - isHostEqual = TestCredentials.DefaultHostOverride == context.TargetHost; - isPemEqual = certChain == context.TargetPem; - return returnValue; - } - private class SslCredentialsTestServiceImpl : TestService.TestServiceBase { public override Task UnaryCall(SimpleRequest request, ServerCallContext context) diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index 5f80d3417a5..4323d40b6d3 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -901,6 +901,21 @@ grpcsharp_server_request_call(grpc_server* server, grpc_completion_queue* cq, &(ctx->request_metadata), cq, cq, ctx); } +/* Native callback dispatcher */ + +typedef int(GPR_CALLTYPE* grpcsharp_native_callback_dispatcher_func)( + void* tag, void* arg0, void* arg1, void* arg2, void* arg3, void* arg4, + void* arg5); + +static grpcsharp_native_callback_dispatcher_func native_callback_dispatcher = + NULL; + +GPR_EXPORT void GPR_CALLTYPE grpcsharp_native_callback_dispatcher_init( + grpcsharp_native_callback_dispatcher_func func) { + GPR_ASSERT(func); + native_callback_dispatcher = func; +} + /* Security */ static char* default_pem_root_certs = NULL; @@ -927,23 +942,18 @@ grpcsharp_override_default_ssl_roots(const char* pem_root_certs) { grpc_set_ssl_roots_override_callback(override_ssl_roots_handler); } -typedef int(GPR_CALLTYPE* grpcsharp_verify_peer_func)(const char* target_host, - const char* target_pem, - void* userdata, - int32_t isDestroy); - static void grpcsharp_verify_peer_destroy_handler(void* userdata) { - grpcsharp_verify_peer_func callback = - (grpcsharp_verify_peer_func)(intptr_t)userdata; - callback(NULL, NULL, NULL, 1); + native_callback_dispatcher(userdata, NULL, + NULL, (void*)1, NULL, + NULL, NULL); } static int grpcsharp_verify_peer_handler(const char* target_host, const char* target_pem, void* userdata) { - grpcsharp_verify_peer_func callback = - (grpcsharp_verify_peer_func)(intptr_t)userdata; - return callback(target_host, target_pem, NULL, 0); + return native_callback_dispatcher(userdata, (void*)target_host, + (void*)target_pem, (void*)0, NULL, + NULL, NULL); } @@ -951,13 +961,13 @@ GPR_EXPORT grpc_channel_credentials* GPR_CALLTYPE grpcsharp_ssl_credentials_create(const char* pem_root_certs, const char* key_cert_pair_cert_chain, const char* key_cert_pair_private_key, - grpcsharp_verify_peer_func verify_peer_func) { + void* verify_peer_callback_tag) { grpc_ssl_pem_key_cert_pair key_cert_pair; verify_peer_options verify_options; verify_peer_options* p_verify_options = NULL; - if (verify_peer_func != NULL) { + if (verify_peer_callback_tag != NULL) { verify_options.verify_peer_callback_userdata = - (void*)(intptr_t)verify_peer_func; + verify_peer_callback_tag; verify_options.verify_peer_destruct = grpcsharp_verify_peer_destroy_handler; verify_options.verify_peer_callback = grpcsharp_verify_peer_handler; @@ -1043,21 +1053,6 @@ grpcsharp_composite_call_credentials_create(grpc_call_credentials* creds1, return grpc_composite_call_credentials_create(creds1, creds2, NULL); } -/* Native callback dispatcher */ - -typedef int(GPR_CALLTYPE* grpcsharp_native_callback_dispatcher_func)( - void* tag, void* arg0, void* arg1, void* arg2, void* arg3, void* arg4, - void* arg5); - -static grpcsharp_native_callback_dispatcher_func native_callback_dispatcher = - NULL; - -GPR_EXPORT void GPR_CALLTYPE grpcsharp_native_callback_dispatcher_init( - grpcsharp_native_callback_dispatcher_func func) { - GPR_ASSERT(func); - native_callback_dispatcher = func; -} - /* Metadata credentials plugin */ GPR_EXPORT void GPR_CALLTYPE grpcsharp_metadata_credentials_notify_from_plugin( diff --git a/templates/src/csharp/Grpc.Core/Internal/native_methods.include b/templates/src/csharp/Grpc.Core/Internal/native_methods.include index 4cb71730529..e8ec4c87b06 100644 --- a/templates/src/csharp/Grpc.Core/Internal/native_methods.include +++ b/templates/src/csharp/Grpc.Core/Internal/native_methods.include @@ -44,7 +44,7 @@ native_method_signatures = [ 'void grpcsharp_channel_args_set_integer(ChannelArgsSafeHandle args, UIntPtr index, string key, int value)', 'void grpcsharp_channel_args_destroy(IntPtr args)', 'void grpcsharp_override_default_ssl_roots(string pemRootCerts)', - 'ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, VerifyPeerCallbackInternal verifyPeerCallback)', + 'ChannelCredentialsSafeHandle grpcsharp_ssl_credentials_create(string pemRootCerts, string keyCertPairCertChain, string keyCertPairPrivateKey, IntPtr verifyPeerCallbackTag)', 'ChannelCredentialsSafeHandle grpcsharp_composite_channel_credentials_create(ChannelCredentialsSafeHandle channelCreds, CallCredentialsSafeHandle callCreds)', 'void grpcsharp_channel_credentials_release(IntPtr credentials)', 'ChannelSafeHandle grpcsharp_insecure_channel_create(string target, ChannelArgsSafeHandle channelArgs)', From 046cd3533b85425631bf3e3be43da8fb77e5d8b3 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Sun, 31 Mar 2019 19:34:50 -0700 Subject: [PATCH 14/30] add license --- src/csharp/Grpc.Core/VerifyPeerContext.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/csharp/Grpc.Core/VerifyPeerContext.cs b/src/csharp/Grpc.Core/VerifyPeerContext.cs index b48984b093c..52a5fabf7c8 100644 --- a/src/csharp/Grpc.Core/VerifyPeerContext.cs +++ b/src/csharp/Grpc.Core/VerifyPeerContext.cs @@ -1,3 +1,21 @@ +#region Copyright notice and license + +// Copyright 2019 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#endregion + namespace Grpc.Core { /// From d297ede96a5cb453e32d40ec22015e67e214942a Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Sun, 31 Mar 2019 19:47:11 -0700 Subject: [PATCH 15/30] honor C core's naming of verify_peer_callback arguments --- src/csharp/Grpc.Core/ChannelCredentials.cs | 4 ++-- src/csharp/Grpc.Core/VerifyPeerContext.cs | 18 +++++++++--------- src/csharp/ext/grpc_csharp_ext.c | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/csharp/Grpc.Core/ChannelCredentials.cs b/src/csharp/Grpc.Core/ChannelCredentials.cs index 2ecd3875e84..fbea40d0a4b 100644 --- a/src/csharp/Grpc.Core/ChannelCredentials.cs +++ b/src/csharp/Grpc.Core/ChannelCredentials.cs @@ -226,7 +226,7 @@ namespace Grpc.Core return VerifyPeerCallbackHandler(arg0, arg1, arg2 != IntPtr.Zero); } - private int VerifyPeerCallbackHandler(IntPtr host, IntPtr pem, bool isDestroy) + private int VerifyPeerCallbackHandler(IntPtr targetName, IntPtr peerPem, bool isDestroy) { if (isDestroy) { @@ -236,7 +236,7 @@ namespace Grpc.Core try { - var context = new VerifyPeerContext(Marshal.PtrToStringAnsi(host), Marshal.PtrToStringAnsi(pem)); + var context = new VerifyPeerContext(Marshal.PtrToStringAnsi(targetName), Marshal.PtrToStringAnsi(peerPem)); return this.verifyPeerCallback(context) ? 0 : 1; } diff --git a/src/csharp/Grpc.Core/VerifyPeerContext.cs b/src/csharp/Grpc.Core/VerifyPeerContext.cs index 52a5fabf7c8..b1dc60f8e27 100644 --- a/src/csharp/Grpc.Core/VerifyPeerContext.cs +++ b/src/csharp/Grpc.Core/VerifyPeerContext.cs @@ -27,22 +27,22 @@ namespace Grpc.Core /// /// Initializes a new instance of the class. /// - /// string containing the host name of the peer. - /// string containing PEM encoded certificate of the peer. - internal VerifyPeerContext(string targetHost, string targetPem) + /// The target name of the peer. + /// The PEM encoded certificate of the peer. + internal VerifyPeerContext(string targetName, string peerPem) { - this.TargetHost = targetHost; - this.TargetPem = targetPem; + this.TargetName = targetName; + this.PeerPem = peerPem; } /// - /// String containing the host name of the peer. + /// The target name of the peer. /// - public string TargetHost { get; } + public string TargetName { get; } /// - /// string containing PEM encoded certificate of the peer. + /// The PEM encoded certificate of the peer. /// - public string TargetPem { get; } + public string PeerPem { get; } } } diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index 4323d40b6d3..b3bb7c668ca 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -948,11 +948,11 @@ static void grpcsharp_verify_peer_destroy_handler(void* userdata) { NULL, NULL); } -static int grpcsharp_verify_peer_handler(const char* target_host, - const char* target_pem, +static int grpcsharp_verify_peer_handler(const char* target_name, + const char* peer_pem, void* userdata) { - return native_callback_dispatcher(userdata, (void*)target_host, - (void*)target_pem, (void*)0, NULL, + return native_callback_dispatcher(userdata, (void*)target_name, + (void*)peer_pem, (void*)0, NULL, NULL, NULL); } From c02aec32554a44b2d8180cd461ec1617ba0b113e Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 1 Apr 2019 00:13:54 -0400 Subject: [PATCH 16/30] clang format code --- src/csharp/ext/grpc_csharp_ext.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index b3bb7c668ca..061a5edc4ab 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -943,20 +943,16 @@ grpcsharp_override_default_ssl_roots(const char* pem_root_certs) { } static void grpcsharp_verify_peer_destroy_handler(void* userdata) { - native_callback_dispatcher(userdata, NULL, - NULL, (void*)1, NULL, - NULL, NULL); + native_callback_dispatcher(userdata, NULL, NULL, (void*)1, NULL, NULL, NULL); } static int grpcsharp_verify_peer_handler(const char* target_name, - const char* peer_pem, - void* userdata) { + const char* peer_pem, void* userdata) { return native_callback_dispatcher(userdata, (void*)target_name, - (void*)peer_pem, (void*)0, NULL, - NULL, NULL); + (void*)peer_pem, (void*)0, NULL, NULL, + NULL); } - GPR_EXPORT grpc_channel_credentials* GPR_CALLTYPE grpcsharp_ssl_credentials_create(const char* pem_root_certs, const char* key_cert_pair_cert_chain, @@ -966,10 +962,8 @@ grpcsharp_ssl_credentials_create(const char* pem_root_certs, verify_peer_options verify_options; verify_peer_options* p_verify_options = NULL; if (verify_peer_callback_tag != NULL) { - verify_options.verify_peer_callback_userdata = - verify_peer_callback_tag; - verify_options.verify_peer_destruct = - grpcsharp_verify_peer_destroy_handler; + verify_options.verify_peer_callback_userdata = verify_peer_callback_tag; + verify_options.verify_peer_destruct = grpcsharp_verify_peer_destroy_handler; verify_options.verify_peer_callback = grpcsharp_verify_peer_handler; p_verify_options = &verify_options; } From 57caf7840766999a1c9e44537730a821cbbacb79 Mon Sep 17 00:00:00 2001 From: Yihua Zhang Date: Mon, 1 Apr 2019 08:48:35 -0700 Subject: [PATCH 17/30] fix ALPN issues. --- .../ssl/ssl_security_connector.cc | 17 ++------ test/core/security/security_connector_test.cc | 43 ++++++++++++++++++- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/core/lib/security/security_connector/ssl/ssl_security_connector.cc b/src/core/lib/security/security_connector/ssl/ssl_security_connector.cc index 8a00bbb82ed..ad492f361aa 100644 --- a/src/core/lib/security/security_connector/ssl/ssl_security_connector.cc +++ b/src/core/lib/security/security_connector/ssl/ssl_security_connector.cc @@ -44,24 +44,15 @@ namespace { grpc_error* ssl_check_peer( const char* peer_name, const tsi_peer* peer, grpc_core::RefCountedPtr* auth_context) { -#if TSI_OPENSSL_ALPN_SUPPORT - /* Check the ALPN if ALPN is supported. */ - const tsi_peer_property* p = - tsi_peer_get_property_by_name(peer, TSI_SSL_ALPN_SELECTED_PROTOCOL); - if (p == nullptr) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "Cannot check peer: missing selected ALPN property."); - } - if (!grpc_chttp2_is_alpn_version_supported(p->value.data, p->value.length)) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "Cannot check peer: invalid ALPN value."); + grpc_error* error = grpc_ssl_check_alpn(peer); + if (error != GRPC_ERROR_NONE) { + return error; } -#endif /* TSI_OPENSSL_ALPN_SUPPORT */ /* Check the peer name if specified. */ if (peer_name != nullptr && !grpc_ssl_host_matches_name(peer, peer_name)) { char* msg; gpr_asprintf(&msg, "Peer name %s is not in peer certificate", peer_name); - grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); + error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); gpr_free(msg); return error; } diff --git a/test/core/security/security_connector_test.cc b/test/core/security/security_connector_test.cc index 2a31763c73c..496f064439c 100644 --- a/test/core/security/security_connector_test.cc +++ b/test/core/security/security_connector_test.cc @@ -36,6 +36,10 @@ #include "src/core/tsi/transport_security.h" #include "test/core/util/test_config.h" +#ifndef TSI_OPENSSL_ALPN_SUPPORT +#define TSI_OPENSSL_ALPN_SUPPORT 1 +#endif + static int check_transport_security_type(const grpc_auth_context* ctx) { grpc_auth_property_iterator it = grpc_auth_context_find_properties_by_name( ctx, GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME); @@ -432,6 +436,43 @@ static void test_default_ssl_roots(void) { gpr_free(roots_env_var_file_path); } +static void test_peer_alpn_check(void) { +#if TSI_OPENSSL_ALPN_SUPPORT + tsi_peer peer; + const char* alpn = "grpc"; + const char* wrong_alpn = "wrong"; + // peer does not have a TSI_SSL_ALPN_SELECTED_PROTOCOL property. + GPR_ASSERT(tsi_construct_peer(1, &peer) == TSI_OK); + GPR_ASSERT(tsi_construct_string_peer_property("wrong peer property name", + alpn, strlen(alpn), + &peer.properties[0]) == TSI_OK); + grpc_error* error = grpc_ssl_check_alpn(&peer); + GPR_ASSERT(error != GRPC_ERROR_NONE); + tsi_peer_destruct(&peer); + GRPC_ERROR_UNREF(error); + // peer has a TSI_SSL_ALPN_SELECTED_PROTOCOL property but with an incorrect + // property value. + GPR_ASSERT(tsi_construct_peer(1, &peer) == TSI_OK); + GPR_ASSERT(tsi_construct_string_peer_property(TSI_SSL_ALPN_SELECTED_PROTOCOL, + wrong_alpn, strlen(wrong_alpn), + &peer.properties[0]) == TSI_OK); + error = grpc_ssl_check_alpn(&peer); + GPR_ASSERT(error != GRPC_ERROR_NONE); + tsi_peer_destruct(&peer); + GRPC_ERROR_UNREF(error); + // peer has a TSI_SSL_ALPN_SELECTED_PROTOCOL property with a correct property + // value. + GPR_ASSERT(tsi_construct_peer(1, &peer) == TSI_OK); + GPR_ASSERT(tsi_construct_string_peer_property(TSI_SSL_ALPN_SELECTED_PROTOCOL, + alpn, strlen(alpn), + &peer.properties[0]) == TSI_OK); + GPR_ASSERT(grpc_ssl_check_alpn(&peer) == GRPC_ERROR_NONE); + tsi_peer_destruct(&peer); +#else + GPR_ASSERT(grpc_ssl_check_alpn(nullptr) == GRPC_ERROR_NONE); +#endif +} + int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); grpc_init(); @@ -443,7 +484,7 @@ int main(int argc, char** argv) { test_cn_and_multiple_sans_and_others_ssl_peer_to_auth_context(); test_ipv6_address_san(); test_default_ssl_roots(); - + test_peer_alpn_check(); grpc_shutdown(); return 0; } From 6ddee61f7567ed000867cdc3238101c087877c99 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 1 Apr 2019 13:31:36 -0700 Subject: [PATCH 18/30] Upgrade linux RBE tests to bazel 0.22 --- tools/internal_ci/linux/grpc_bazel_on_foundry_base.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_bazel_on_foundry_base.sh b/tools/internal_ci/linux/grpc_bazel_on_foundry_base.sh index 863b43a1728..1882fe213fa 100755 --- a/tools/internal_ci/linux/grpc_bazel_on_foundry_base.sh +++ b/tools/internal_ci/linux/grpc_bazel_on_foundry_base.sh @@ -23,7 +23,7 @@ cp ${KOKORO_GFILE_DIR}/GrpcTesting-d0eeee2db331.json ${KOKORO_KEYSTORE_DIR}/4321 # Download bazel temp_dir="$(mktemp -d)" -wget -q https://github.com/bazelbuild/bazel/releases/download/0.20.0/bazel-0.20.0-linux-x86_64 -O "${temp_dir}/bazel" +wget -q https://github.com/bazelbuild/bazel/releases/download/0.22.0/bazel-0.22.0-linux-x86_64 -O "${temp_dir}/bazel" chmod 755 "${temp_dir}/bazel" export PATH="${temp_dir}:${PATH}" # This should show ${temp_dir}/bazel From 39b40cbccbe8145dba88583bbdac0ff88a111198 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 1 Apr 2019 13:37:35 -0700 Subject: [PATCH 19/30] Add guard to the tv_nsec field of gpr_now return value --- src/core/lib/gpr/time_posix.cc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/core/lib/gpr/time_posix.cc b/src/core/lib/gpr/time_posix.cc index 1b3e36486fc..b5a47673676 100644 --- a/src/core/lib/gpr/time_posix.cc +++ b/src/core/lib/gpr/time_posix.cc @@ -108,6 +108,9 @@ static gpr_timespec now_impl(gpr_clock_type clock) { now.clock_type = clock; switch (clock) { case GPR_CLOCK_REALTIME: + // gettimeofday(...) function may return with a value whose tv_usec is + // greater than 1e6 on iOS The case is resolved with the guard at end of + // this function. gettimeofday(&now_tv, nullptr); now.tv_sec = now_tv.tv_sec; now.tv_nsec = now_tv.tv_usec * 1000; @@ -124,6 +127,16 @@ static gpr_timespec now_impl(gpr_clock_type clock) { abort(); } + // Guard the tv_nsec field in valid range for all clock types + while (GPR_UNLIKELY(now.tv_nsec >= 1e9)) { + now.tv_sec++; + now.tv_nsec -= 1e9; + } + while (GPR_UNLIKELY(now.tv_nsec < 0)) { + now.tv_sec--; + now.tv_nsec += 1e9; + } + return now; } #endif From 6da82f08bda80c4c167047bd20ef81655c27551b Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 1 Apr 2019 17:14:30 -0400 Subject: [PATCH 20/30] add job to give early warning about incompatible bazel changes --- .../grpc_bazel_rbe_incompatible_changes.sh | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 tools/internal_ci/linux/grpc_bazel_rbe_incompatible_changes.sh diff --git a/tools/internal_ci/linux/grpc_bazel_rbe_incompatible_changes.sh b/tools/internal_ci/linux/grpc_bazel_rbe_incompatible_changes.sh new file mode 100644 index 00000000000..9c3712a07da --- /dev/null +++ b/tools/internal_ci/linux/grpc_bazel_rbe_incompatible_changes.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# Copyright 2017 gRPC authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -ex + +# TODO(jtattermusch): use the latest version of bazel + +# Use --all_incompatible_changes to give an early warning about future +# bazel incompatibilities. +EXTRA_FLAGS="--config=opt --cache_test_results=no --all_incompatible_changes" +github/grpc/tools/internal_ci/linux/grpc_bazel_on_foundry_base.sh "${EXTRA_FLAGS}" From ee4e4bead3d604c37484a657e692d61c9527f70b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 1 Apr 2019 14:55:29 -0700 Subject: [PATCH 21/30] pick_first: don't go into TRANSIENT_FAILURE upon empty update when in IDLE. --- .../lb_policy/pick_first/pick_first.cc | 26 ++++++++++++++----- test/cpp/end2end/client_lb_end2end_test.cc | 26 +++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index 86c6b25ac63..3da0b59f279 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -188,8 +188,14 @@ void PickFirst::ShutdownLocked() { void PickFirst::ExitIdleLocked() { if (idle_) { idle_ = false; - if (subchannel_list_ != nullptr && - subchannel_list_->num_subchannels() > 0) { + if (subchannel_list_ == nullptr || + subchannel_list_->num_subchannels() == 0) { + grpc_error* error = + GRPC_ERROR_CREATE_FROM_STATIC_STRING("No addresses to connect to"); + channel_control_helper()->UpdateState( + GRPC_CHANNEL_TRANSIENT_FAILURE, GRPC_ERROR_REF(error), + UniquePtr(New(error))); + } else { subchannel_list_->subchannel(0) ->CheckConnectivityStateAndStartWatchingLocked(); } @@ -253,13 +259,19 @@ void PickFirst::UpdateLocked(UpdateArgs args) { grpc_channel_args_destroy(new_args); if (subchannel_list->num_subchannels() == 0) { // Empty update or no valid subchannels. Unsubscribe from all current - // subchannels and put the channel in TRANSIENT_FAILURE. + // subchannels. subchannel_list_ = std::move(subchannel_list); // Empty list. selected_ = nullptr; - grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Empty update"); - channel_control_helper()->UpdateState( - GRPC_CHANNEL_TRANSIENT_FAILURE, GRPC_ERROR_REF(error), - UniquePtr(New(error))); + // If not idle, put the channel in TRANSIENT_FAILURE. + // (If we are idle, then this will happen in ExitIdleLocked() if we + // haven't gotten a non-empty update by the time the application tries + // to start a new call.) + if (!idle_) { + grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Empty update"); + channel_control_helper()->UpdateState( + GRPC_CHANNEL_TRANSIENT_FAILURE, GRPC_ERROR_REF(error), + UniquePtr(New(error))); + } return; } // If one of the subchannels in the new list is already in state diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index 4244690d76b..77f9db94acc 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -940,6 +940,32 @@ TEST_F(ClientLbEnd2endTest, PickFirstPendingUpdateAndSelectedSubchannelFails) { WaitForServer(stub, 1, DEBUG_LOCATION, true /* ignore_failure */); } +TEST_F(ClientLbEnd2endTest, PickFirstStaysIdleUponEmptyUpdate) { + // Start server, send RPC, and make sure channel is READY. + const int kNumServers = 1; + StartServers(kNumServers); + auto channel = BuildChannel(""); // pick_first is the default. + auto stub = BuildStub(channel); + SetNextResolution(GetServersPorts()); + CheckRpcSendOk(stub, DEBUG_LOCATION); + EXPECT_EQ(channel->GetState(false), GRPC_CHANNEL_READY); + // Stop server. Channel should go into state IDLE. + servers_[0]->Shutdown(); + EXPECT_TRUE(WaitForChannelNotReady(channel.get())); + EXPECT_EQ(channel->GetState(false), GRPC_CHANNEL_IDLE); + // Now send resolver update that includes no addresses. Channel + // should stay in state IDLE. + SetNextResolution({}); + EXPECT_FALSE(channel->WaitForStateChange( + GRPC_CHANNEL_IDLE, grpc_timeout_seconds_to_deadline(3))); + // Now bring the backend back up and send a non-empty resolver update, + // and then try to send an RPC. Channel should go back into state READY. + StartServer(0); + SetNextResolution(GetServersPorts()); + CheckRpcSendOk(stub, DEBUG_LOCATION); + EXPECT_EQ(channel->GetState(false), GRPC_CHANNEL_READY); +} + TEST_F(ClientLbEnd2endTest, RoundRobin) { // Start servers and send one RPC per server. const int kNumServers = 3; From ac7c73605734de136d88b3f5dde8545a468cebe3 Mon Sep 17 00:00:00 2001 From: Karthik Ravi Shankar Date: Mon, 1 Apr 2019 14:34:27 -0700 Subject: [PATCH 22/30] Fix BUILD.gn file https://github.com/grpc/grpc/pull/18345 did not add a file to BUILD.gn causing generate_projects.sh to fail causing this error. Fixing it by running the generate_projects.sh script. --- BUILD.gn | 1 + 1 file changed, 1 insertion(+) diff --git a/BUILD.gn b/BUILD.gn index 4e0bdb6586c..7ceea3c3b63 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1076,6 +1076,7 @@ config("grpc_config") { "include/grpcpp/impl/server_initializer_impl.h", "include/grpcpp/impl/service_type.h", "include/grpcpp/resource_quota.h", + "include/grpcpp/resource_quota_impl.h", "include/grpcpp/security/auth_context.h", "include/grpcpp/security/auth_metadata_processor.h", "include/grpcpp/security/credentials.h", From 7e6b5db90de63c820e80440502aa1427bcd72ed9 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 1 Apr 2019 15:34:41 -0700 Subject: [PATCH 23/30] Resolve merge issue --- BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/BUILD b/BUILD index f24e986329f..003914a3c5f 100644 --- a/BUILD +++ b/BUILD @@ -369,7 +369,6 @@ grpc_cc_library( "grpc++_codegen_base", "grpc++_codegen_base_src", "grpc++_codegen_proto", - "grpc_cfstream", ], ) From e69406008fc0c7b7a26c5d0bf79bc7b6efce503b Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Mon, 1 Apr 2019 12:30:42 -0700 Subject: [PATCH 24/30] Explicitly set case_insensitive_enum_parsing = true when parsing JSON --- src/cpp/server/channelz/channelz_service.cc | 27 ++++++++++----------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/cpp/server/channelz/channelz_service.cc b/src/cpp/server/channelz/channelz_service.cc index c44a9ac0de6..0409991fe67 100644 --- a/src/cpp/server/channelz/channelz_service.cc +++ b/src/cpp/server/channelz/channelz_service.cc @@ -24,6 +24,12 @@ #include namespace grpc { +grpc::protobuf::util::Status ParseJson(const char* json_str, + grpc::protobuf::Message* message) { + grpc::protobuf::json::JsonParseOptions options; + options.case_insensitive_enum_parsing = true; + return grpc::protobuf::json::JsonStringToMessage(json_str, message, options); +} Status ChannelzService::GetTopChannels( ServerContext* unused, const channelz::v1::GetTopChannelsRequest* request, @@ -33,8 +39,7 @@ Status ChannelzService::GetTopChannels( return Status(StatusCode::INTERNAL, "grpc_channelz_get_top_channels returned null"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); @@ -50,8 +55,7 @@ Status ChannelzService::GetServers( return Status(StatusCode::INTERNAL, "grpc_channelz_get_servers returned null"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); @@ -67,8 +71,7 @@ Status ChannelzService::GetServer(ServerContext* unused, return Status(StatusCode::INTERNAL, "grpc_channelz_get_server returned null"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); @@ -85,8 +88,7 @@ Status ChannelzService::GetServerSockets( return Status(StatusCode::INTERNAL, "grpc_channelz_get_server_sockets returned null"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); @@ -101,8 +103,7 @@ Status ChannelzService::GetChannel( if (json_str == nullptr) { return Status(StatusCode::NOT_FOUND, "No object found for that ChannelId"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); @@ -118,8 +119,7 @@ Status ChannelzService::GetSubchannel( return Status(StatusCode::NOT_FOUND, "No object found for that SubchannelId"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); @@ -134,8 +134,7 @@ Status ChannelzService::GetSocket(ServerContext* unused, if (json_str == nullptr) { return Status(StatusCode::NOT_FOUND, "No object found for that SocketId"); } - grpc::protobuf::util::Status s = - grpc::protobuf::json::JsonStringToMessage(json_str, response); + grpc::protobuf::util::Status s = ParseJson(json_str, response); gpr_free(json_str); if (!s.ok()) { return Status(StatusCode::INTERNAL, s.ToString()); From f9a9639088b258f707ed1616b5044787b65ac698 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 1 Apr 2019 15:58:33 -0700 Subject: [PATCH 25/30] generate projects --- BUILD.gn | 1 + 1 file changed, 1 insertion(+) diff --git a/BUILD.gn b/BUILD.gn index 4e0bdb6586c..7ceea3c3b63 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1076,6 +1076,7 @@ config("grpc_config") { "include/grpcpp/impl/server_initializer_impl.h", "include/grpcpp/impl/service_type.h", "include/grpcpp/resource_quota.h", + "include/grpcpp/resource_quota_impl.h", "include/grpcpp/security/auth_context.h", "include/grpcpp/security/auth_metadata_processor.h", "include/grpcpp/security/credentials.h", From 69bbf86d147f988d142914de0da8a0b6eac9afba Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 1 Apr 2019 16:01:49 -0700 Subject: [PATCH 26/30] Add a dummy function to grpc cfstream library --- src/core/lib/iomgr/cfstream_handle.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/lib/iomgr/cfstream_handle.cc b/src/core/lib/iomgr/cfstream_handle.cc index cf21c4fc511..2fda160f68a 100644 --- a/src/core/lib/iomgr/cfstream_handle.cc +++ b/src/core/lib/iomgr/cfstream_handle.cc @@ -192,4 +192,11 @@ void CFStreamHandle::Unref(const char* file, int line, const char* reason) { } } +#else + +/* Creating a dummy function so that the grpc_cfstream library will be + * non-empty. + */ +void CFStreamDummy() {} + #endif From cc44a729db8bec1f991be19f49fa86b681b5f49b Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 1 Apr 2019 20:42:02 -0400 Subject: [PATCH 27/30] improve doc comment --- src/csharp/Grpc.Core/ChannelCredentials.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/csharp/Grpc.Core/ChannelCredentials.cs b/src/csharp/Grpc.Core/ChannelCredentials.cs index fbea40d0a4b..dcbe9f3599b 100644 --- a/src/csharp/Grpc.Core/ChannelCredentials.cs +++ b/src/csharp/Grpc.Core/ChannelCredentials.cs @@ -109,8 +109,12 @@ namespace Grpc.Core /// /// Callback invoked with the expected targetHost and the peer's certificate. /// If false is returned by this callback then it is treated as a - /// verification failure. Invocation of the callback is blocking, so any + /// verification failure and the attempted connection will fail. + /// Invocation of the callback is blocking, so any /// implementation should be light-weight. + /// Note that the callback can potentially be invoked multiple times, + /// concurrently from different threads (e.g. when multiple connections + /// are being created for the same credentials). /// /// The associated with the callback /// true if verification succeeded, false otherwise. From c9974ab6c9090bb25e41f80a2f0fb8a12beb9f14 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 1 Apr 2019 20:55:50 -0400 Subject: [PATCH 28/30] refactor grpcsharp_ssl_credentials_create --- src/csharp/ext/grpc_csharp_ext.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index 061a5edc4ab..91d3957dbf0 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -960,25 +960,29 @@ grpcsharp_ssl_credentials_create(const char* pem_root_certs, void* verify_peer_callback_tag) { grpc_ssl_pem_key_cert_pair key_cert_pair; verify_peer_options verify_options; - verify_peer_options* p_verify_options = NULL; - if (verify_peer_callback_tag != NULL) { - verify_options.verify_peer_callback_userdata = verify_peer_callback_tag; - verify_options.verify_peer_destruct = grpcsharp_verify_peer_destroy_handler; - verify_options.verify_peer_callback = grpcsharp_verify_peer_handler; - p_verify_options = &verify_options; - } + grpc_ssl_pem_key_cert_pair* key_cert_pair_ptr = NULL; + verify_peer_options* verify_options_ptr = NULL; if (key_cert_pair_cert_chain || key_cert_pair_private_key) { + memset(&key_cert_pair, 0, sizeof(key_cert_pair)); key_cert_pair.cert_chain = key_cert_pair_cert_chain; key_cert_pair.private_key = key_cert_pair_private_key; - return grpc_ssl_credentials_create(pem_root_certs, &key_cert_pair, - p_verify_options, NULL); + key_cert_pair_ptr = &key_cert_pair; } else { GPR_ASSERT(!key_cert_pair_cert_chain); GPR_ASSERT(!key_cert_pair_private_key); - return grpc_ssl_credentials_create(pem_root_certs, NULL, p_verify_options, - NULL); } + + if (verify_peer_callback_tag != NULL) { + memset(&verify_options, 0, sizeof(verify_peer_options)); + verify_options.verify_peer_callback_userdata = verify_peer_callback_tag; + verify_options.verify_peer_destruct = grpcsharp_verify_peer_destroy_handler; + verify_options.verify_peer_callback = grpcsharp_verify_peer_handler; + verify_options_ptr = &verify_options; + } + + return grpc_ssl_credentials_create(pem_root_certs, key_cert_pair_ptr, + verify_options_ptr, NULL); } GPR_EXPORT void GPR_CALLTYPE From 89a9e9d1dd000dff80719c460ed73322a388bbfb Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 2 Apr 2019 14:33:11 -0700 Subject: [PATCH 29/30] code review changes --- .../filters/client_channel/client_channel.cc | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index c856199d560..f638423a9de 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -107,12 +107,14 @@ class ExternalConnectivityWatcher { int size() const; ExternalConnectivityWatcher* Lookup(grpc_closure* on_complete) const; - void Append(ExternalConnectivityWatcher* watcher); - void Remove(ExternalConnectivityWatcher* watcher); + void Add(ExternalConnectivityWatcher* watcher); + void Remove(const ExternalConnectivityWatcher* watcher); private: - // head_ is guarded by its own mutex, since the size of the list needs - // to be grabbed immediately without polling on a CQ. + // head_ is guarded by a mutex, since the size() method needs to + // iterate over the list, and it's called from the C-core API + // function grpc_channel_num_external_connectivity_watchers(), which + // is synchronous and therefore cannot run in the combiner. mutable gpr_mu mu_; ExternalConnectivityWatcher* head_ = nullptr; }; @@ -260,9 +262,9 @@ ExternalConnectivityWatcher* ExternalConnectivityWatcher::WatcherList::Lookup( return w; } -void ExternalConnectivityWatcher::WatcherList::Append( +void ExternalConnectivityWatcher::WatcherList::Add( ExternalConnectivityWatcher* watcher) { - GPR_ASSERT(!Lookup(watcher->on_complete_)); + GPR_ASSERT(Lookup(watcher->on_complete_) == nullptr); MutexLock lock(&mu_); GPR_ASSERT(watcher->next_ == nullptr); watcher->next_ = head_; @@ -270,7 +272,7 @@ void ExternalConnectivityWatcher::WatcherList::Append( } void ExternalConnectivityWatcher::WatcherList::Remove( - ExternalConnectivityWatcher* watcher) { + const ExternalConnectivityWatcher* watcher) { MutexLock lock(&mu_); if (watcher == head_) { head_ = watcher->next_; @@ -333,7 +335,6 @@ void ExternalConnectivityWatcher::WatchConnectivityStateLocked( self->chand_->external_connectivity_watcher_list->Lookup( self->on_complete_); if (found != nullptr) { - GPR_ASSERT(found->on_complete_ == self->on_complete_); grpc_connectivity_state_notify_on_state_change( &found->chand_->state_tracker, nullptr, &found->my_closure_); } @@ -341,7 +342,7 @@ void ExternalConnectivityWatcher::WatchConnectivityStateLocked( return; } // New watcher. - self->chand_->external_connectivity_watcher_list->Append(self); + self->chand_->external_connectivity_watcher_list->Add(self); // This assumes that the closure is scheduled on the ExecCtx scheduler // and that GRPC_CLOSURE_RUN would run the closure immediately. GRPC_CLOSURE_RUN(self->watcher_timer_init_, GRPC_ERROR_NONE); From adf5f872846ce2d7f03193a72fdf04310ebd25ec Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 2 Apr 2019 15:16:29 -0700 Subject: [PATCH 30/30] more code review changes --- src/core/ext/filters/client_channel/client_channel.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index f638423a9de..34a24e2c712 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -127,7 +127,7 @@ class ExternalConnectivityWatcher { ~ExternalConnectivityWatcher(); private: - static void OnExternalWatchCompleteLocked(void* arg, grpc_error* error); + static void OnWatchCompleteLocked(void* arg, grpc_error* error); static void WatchConnectivityStateLocked(void* arg, grpc_error* ignored); channel_data* chand_; @@ -314,8 +314,8 @@ ExternalConnectivityWatcher::~ExternalConnectivityWatcher() { GRPC_CHANNEL_STACK_UNREF(chand_->owning_stack, "ExternalConnectivityWatcher"); } -void ExternalConnectivityWatcher::OnExternalWatchCompleteLocked( - void* arg, grpc_error* error) { +void ExternalConnectivityWatcher::OnWatchCompleteLocked(void* arg, + grpc_error* error) { ExternalConnectivityWatcher* self = static_cast(arg); grpc_closure* on_complete = self->on_complete_; @@ -346,7 +346,7 @@ void ExternalConnectivityWatcher::WatchConnectivityStateLocked( // This assumes that the closure is scheduled on the ExecCtx scheduler // and that GRPC_CLOSURE_RUN would run the closure immediately. GRPC_CLOSURE_RUN(self->watcher_timer_init_, GRPC_ERROR_NONE); - GRPC_CLOSURE_INIT(&self->my_closure_, OnExternalWatchCompleteLocked, self, + GRPC_CLOSURE_INIT(&self->my_closure_, OnWatchCompleteLocked, self, grpc_combiner_scheduler(self->chand_->combiner)); grpc_connectivity_state_notify_on_state_change( &self->chand_->state_tracker, self->state_, &self->my_closure_);