From bc20975f21d6b7a9dbf888fd9f9c283d5c9f5e6f Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 15 May 2018 11:18:56 -0700 Subject: [PATCH 1/5] Fix sanity script --- .../run_tests/sanity/core_banned_functions.py | 43 ++++++++++--------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/tools/run_tests/sanity/core_banned_functions.py b/tools/run_tests/sanity/core_banned_functions.py index 989990542e3..0b3540a0e28 100755 --- a/tools/run_tests/sanity/core_banned_functions.py +++ b/tools/run_tests/sanity/core_banned_functions.py @@ -23,35 +23,36 @@ os.chdir(os.path.join(os.path.dirname(sys.argv[0]), '../../..')) # map of banned function signature to whitelist BANNED_EXCEPT = { - 'grpc_resource_quota_ref(': ['src/core/lib/iomgr/resource_quota.c'], - 'grpc_resource_quota_unref(': ['src/core/lib/iomgr/resource_quota.c'], - 'grpc_slice_buffer_destroy(': ['src/core/lib/slice/slice_buffer.c'], - 'grpc_slice_buffer_reset_and_unref(': ['src/core/lib/slice/slice_buffer.c'], - 'grpc_slice_ref(': ['src/core/lib/slice/slice.c'], - 'grpc_slice_unref(': ['src/core/lib/slice/slice.c'], - 'grpc_error_create(': ['src/core/lib/iomgr/error.c'], - 'grpc_error_ref(': ['src/core/lib/iomgr/error.c'], - 'grpc_error_unref(': ['src/core/lib/iomgr/error.c'], - 'grpc_os_error(': ['src/core/lib/iomgr/error.c'], - 'grpc_wsa_error(': ['src/core/lib/iomgr/error.c'], - 'grpc_log_if_error(': ['src/core/lib/iomgr/error.c'], - 'grpc_slice_malloc(': ['src/core/lib/slice/slice.c'], - 'grpc_closure_create(': ['src/core/lib/iomgr/closure.c'], - 'grpc_closure_init(': ['src/core/lib/iomgr/closure.c'], - 'grpc_closure_sched(': ['src/core/lib/iomgr/closure.c'], - 'grpc_closure_run(': ['src/core/lib/iomgr/closure.c'], - 'grpc_closure_list_sched(': ['src/core/lib/iomgr/closure.c'], + 'grpc_resource_quota_ref(': ['src/core/lib/iomgr/resource_quota.cc'], + 'grpc_resource_quota_unref(': ['src/core/lib/iomgr/resource_quota.cc'], + 'grpc_slice_buffer_destroy(': ['src/core/lib/slice/slice_buffer.cc'], + 'grpc_slice_buffer_reset_and_unref(': ['src/core/lib/slice/slice_buffer.cc'], + 'grpc_slice_ref(': ['src/core/lib/slice/slice.cc'], + 'grpc_slice_unref(': ['src/core/lib/slice/slice.cc'], + 'grpc_error_create(': ['src/core/lib/iomgr/error.cc'], + 'grpc_error_ref(': ['src/core/lib/iomgr/error.cc'], + 'grpc_error_unref(': ['src/core/lib/iomgr/error.cc'], + 'grpc_os_error(': ['src/core/lib/iomgr/error.cc'], + 'grpc_wsa_error(': ['src/core/lib/iomgr/error.cc'], + 'grpc_log_if_error(': ['src/core/lib/iomgr/error.cc'], + 'grpc_slice_malloc(': ['src/core/lib/slice/slice.cc'], + 'grpc_closure_create(': ['src/core/lib/iomgr/closure.cc'], + 'grpc_closure_init(': ['src/core/lib/iomgr/closure.cc'], + 'grpc_closure_sched(': ['src/core/lib/iomgr/closure.cc'], + 'grpc_closure_run(': ['src/core/lib/iomgr/closure.cc'], + 'grpc_closure_list_sched(': ['src/core/lib/iomgr/closure.cc'], 'gpr_getenv_silent(': [ - 'src/core/lib/gpr/log.c', 'src/core/lib/gpr/env_linux.c', - 'src/core/lib/gpr/env_posix.c', 'src/core/lib/gpr/env_windows.c' + 'src/core/lib/gpr/log.cc', 'src/core/lib/gpr/env_linux.cc', + 'src/core/lib/gpr/env_posix.cc', 'src/core/lib/gpr/env_windows.cc' ], } errors = 0 for root, dirs, files in os.walk('src/core'): + if root.startswith('src/core/tsi'): continue for filename in files: path = os.path.join(root, filename) - if os.path.splitext(path)[1] != '.c': continue + if os.path.splitext(path)[1] != '.cc': continue with open(path) as f: text = f.read() for banned, exceptions in BANNED_EXCEPT.items(): From 98c1de07e6463d34939f51c009fae18daaebdf15 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 15 May 2018 11:20:30 -0700 Subject: [PATCH 2/5] Stop using banned functions --- src/core/ext/filters/http/client_authority_filter.cc | 7 ++++--- .../ext/transport/cronet/transport/cronet_transport.cc | 2 +- src/core/lib/iomgr/tcp_client_posix.cc | 2 +- .../security/security_connector/alts_security_connector.cc | 2 +- src/core/lib/transport/byte_stream.cc | 2 +- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc index 1f57ab5ce62..63b9150aec3 100644 --- a/src/core/ext/filters/http/client_authority_filter.cc +++ b/src/core/ext/filters/http/client_authority_filter.cc @@ -59,8 +59,9 @@ void authority_start_transport_stream_op_batch( initial_metadata->idx.named.authority == nullptr) { grpc_error* error = grpc_metadata_batch_add_head( initial_metadata, &calld->authority_storage, - grpc_mdelem_from_slices(GRPC_MDSTR_AUTHORITY, - grpc_slice_ref(chand->default_authority))); + grpc_mdelem_from_slices( + GRPC_MDSTR_AUTHORITY, + grpc_slice_ref_internal(chand->default_authority))); if (error != GRPC_ERROR_NONE) { grpc_transport_stream_op_batch_finish_with_failure(batch, error, calld->call_combiner); @@ -110,7 +111,7 @@ grpc_error* init_channel_elem(grpc_channel_element* elem, /* Destructor for channel data */ void destroy_channel_elem(grpc_channel_element* elem) { channel_data* chand = static_cast(elem->channel_data); - grpc_slice_unref(chand->default_authority); + grpc_slice_unref_internal(chand->default_authority); } } // namespace diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index 0b88ff7afed..420c2d13e1d 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -736,7 +736,7 @@ static void convert_metadata_to_cronet_headers( if (grpc_is_binary_header(GRPC_MDKEY(mdelem))) { grpc_slice wire_value = grpc_chttp2_base64_encode(GRPC_MDVALUE(mdelem)); value = grpc_slice_to_c_string(wire_value); - grpc_slice_unref(wire_value); + grpc_slice_unref_internal(wire_value); } else { value = grpc_slice_to_c_string(GRPC_MDVALUE(mdelem)); } diff --git a/src/core/lib/iomgr/tcp_client_posix.cc b/src/core/lib/iomgr/tcp_client_posix.cc index 6144d389f7c..f987d31ac1a 100644 --- a/src/core/lib/iomgr/tcp_client_posix.cc +++ b/src/core/lib/iomgr/tcp_client_posix.cc @@ -233,7 +233,7 @@ finish: error = grpc_error_set_str(error, GRPC_ERROR_STR_TARGET_ADDRESS, addr_str_slice /* takes ownership */); } else { - grpc_slice_unref(addr_str_slice); + grpc_slice_unref_internal(addr_str_slice); } if (done) { // This is safe even outside the lock, because "done", the sentinel, is diff --git a/src/core/lib/security/security_connector/alts_security_connector.cc b/src/core/lib/security/security_connector/alts_security_connector.cc index 5ff7d7938bf..46b98ff9a82 100644 --- a/src/core/lib/security/security_connector/alts_security_connector.cc +++ b/src/core/lib/security/security_connector/alts_security_connector.cc @@ -133,7 +133,7 @@ grpc_security_status grpc_alts_auth_context_from_tsi_peer( rpc_versions_prop->value.data, rpc_versions_prop->value.length); bool decode_result = grpc_gcp_rpc_protocol_versions_decode(slice, &peer_versions); - grpc_slice_unref(slice); + grpc_slice_unref_internal(slice); if (!decode_result) { gpr_log(GPR_ERROR, "Invalid peer rpc protocol versions."); return GRPC_SECURITY_ERROR; diff --git a/src/core/lib/transport/byte_stream.cc b/src/core/lib/transport/byte_stream.cc index cb15a71a912..16b85ca0db0 100644 --- a/src/core/lib/transport/byte_stream.cc +++ b/src/core/lib/transport/byte_stream.cc @@ -45,7 +45,7 @@ SliceBufferByteStream::SliceBufferByteStream(grpc_slice_buffer* slice_buffer, SliceBufferByteStream::~SliceBufferByteStream() {} void SliceBufferByteStream::Orphan() { - grpc_slice_buffer_destroy(&backing_buffer_); + grpc_slice_buffer_destroy_internal(&backing_buffer_); GRPC_ERROR_UNREF(shutdown_error_); // Note: We do not actually delete the object here, since // SliceBufferByteStream is usually allocated as part of a larger From 6616fab539971befd391322f6b76aedd7a53d7f5 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 15 May 2018 11:22:32 -0700 Subject: [PATCH 3/5] Sanity check the sanity check lol --- tools/run_tests/sanity/core_banned_functions.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/run_tests/sanity/core_banned_functions.py b/tools/run_tests/sanity/core_banned_functions.py index 0b3540a0e28..714c4b5c77f 100755 --- a/tools/run_tests/sanity/core_banned_functions.py +++ b/tools/run_tests/sanity/core_banned_functions.py @@ -26,7 +26,8 @@ BANNED_EXCEPT = { 'grpc_resource_quota_ref(': ['src/core/lib/iomgr/resource_quota.cc'], 'grpc_resource_quota_unref(': ['src/core/lib/iomgr/resource_quota.cc'], 'grpc_slice_buffer_destroy(': ['src/core/lib/slice/slice_buffer.cc'], - 'grpc_slice_buffer_reset_and_unref(': ['src/core/lib/slice/slice_buffer.cc'], + 'grpc_slice_buffer_reset_and_unref(': + ['src/core/lib/slice/slice_buffer.cc'], 'grpc_slice_ref(': ['src/core/lib/slice/slice.cc'], 'grpc_slice_unref(': ['src/core/lib/slice/slice.cc'], 'grpc_error_create(': ['src/core/lib/iomgr/error.cc'], @@ -48,9 +49,11 @@ BANNED_EXCEPT = { } errors = 0 +num_files = 0 for root, dirs, files in os.walk('src/core'): if root.startswith('src/core/tsi'): continue for filename in files: + num_files += 1 path = os.path.join(root, filename) if os.path.splitext(path)[1] != '.cc': continue with open(path) as f: @@ -62,3 +65,4 @@ for root, dirs, files in os.walk('src/core'): errors += 1 assert errors == 0 +assert num_files > 300 # we definitely have more than 300 files From bd11a12169dad9b4e0131bdf09732e771b600e5e Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 15 May 2018 13:46:20 -0700 Subject: [PATCH 4/5] Reviewer feedback and build fixes --- src/core/lib/iomgr/tcp_client_posix.cc | 1 + .../security/security_connector/alts_security_connector.cc | 1 + tools/run_tests/sanity/core_banned_functions.py | 4 ++++ 3 files changed, 6 insertions(+) diff --git a/src/core/lib/iomgr/tcp_client_posix.cc b/src/core/lib/iomgr/tcp_client_posix.cc index f987d31ac1a..900c0565753 100644 --- a/src/core/lib/iomgr/tcp_client_posix.cc +++ b/src/core/lib/iomgr/tcp_client_posix.cc @@ -45,6 +45,7 @@ #include "src/core/lib/iomgr/tcp_posix.h" #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/iomgr/unix_sockets_posix.h" +#include "src/core/lib/slice/slice_internal.h" extern grpc_core::TraceFlag grpc_tcp_trace; diff --git a/src/core/lib/security/security_connector/alts_security_connector.cc b/src/core/lib/security/security_connector/alts_security_connector.cc index 46b98ff9a82..35a787871a8 100644 --- a/src/core/lib/security/security_connector/alts_security_connector.cc +++ b/src/core/lib/security/security_connector/alts_security_connector.cc @@ -30,6 +30,7 @@ #include "src/core/lib/security/credentials/alts/alts_credentials.h" #include "src/core/lib/security/transport/security_handshaker.h" +#include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/transport/transport.h" #include "src/core/tsi/alts/handshaker/alts_tsi_handshaker.h" diff --git a/tools/run_tests/sanity/core_banned_functions.py b/tools/run_tests/sanity/core_banned_functions.py index 714c4b5c77f..e7eb8e3909e 100755 --- a/tools/run_tests/sanity/core_banned_functions.py +++ b/tools/run_tests/sanity/core_banned_functions.py @@ -65,4 +65,8 @@ for root, dirs, files in os.walk('src/core'): errors += 1 assert errors == 0 +# this check comes about from this issue: +# https://github.com/grpc/grpc/issues/15381 +# basically, a change rendered this script useless and we did not realize. +# This dumb check ensures that type of issue doesn't occur again. assert num_files > 300 # we definitely have more than 300 files From f02d33cd8e5e74d8f1a31beb62048b46c1138505 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 15 May 2018 13:55:24 -0700 Subject: [PATCH 5/5] Clean up phrasing from a hastily written comment --- tools/run_tests/sanity/core_banned_functions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/run_tests/sanity/core_banned_functions.py b/tools/run_tests/sanity/core_banned_functions.py index e7eb8e3909e..1d3f2eba8a6 100755 --- a/tools/run_tests/sanity/core_banned_functions.py +++ b/tools/run_tests/sanity/core_banned_functions.py @@ -65,8 +65,8 @@ for root, dirs, files in os.walk('src/core'): errors += 1 assert errors == 0 -# this check comes about from this issue: +# This check comes about from this issue: # https://github.com/grpc/grpc/issues/15381 -# basically, a change rendered this script useless and we did not realize. -# This dumb check ensures that type of issue doesn't occur again. +# Basically, a change rendered this script useless and we did not realize it. +# This dumb check ensures that this type of issue doesn't occur again. assert num_files > 300 # we definitely have more than 300 files