Merge pull request #15384 from ncteisen/sanity

Fix Sanity Test for Core Banned Functions

6 files changed, 40 insertions, 28 deletions

diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc
index 1f57ab5ce6..63b9150aec 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<channel_data*>(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 0b88ff7afe..420c2d13e1 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 6144d389f7..900c056575 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;
 
@@ -233,7 +234,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 5ff7d7938b..35a787871a 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"
 
@@ -133,7 +134,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 cb15a71a91..16b85ca0db 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
diff --git a/tools/run_tests/sanity/core_banned_functions.py b/tools/run_tests/sanity/core_banned_functions.py
index 989990542e..1d3f2eba8a 100755
--- a/tools/run_tests/sanity/core_banned_functions.py
+++ b/tools/run_tests/sanity/core_banned_functions.py
@@ -23,35 +23,39 @@ 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
+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] != '.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():
@@ -61,3 +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 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