Merge pull request #587 from ctiller/chex2

Fix use-after-free.

13 files changed, 52 insertions, 38 deletions

diff --git a/src/core/security/credentials.c b/src/core/security/credentials.c
index b2e0fd215a..60e82d9dfa 100644
--- a/src/core/security/credentials.c
+++ b/src/core/security/credentials.c
@@ -313,7 +313,7 @@ static void oauth2_token_fetcher_destroy(grpc_credentials *creds) {
     grpc_mdelem_unref(c->access_token_md);
   }
   gpr_mu_destroy(&c->mu);
-  grpc_mdctx_orphan(c->md_ctx);
+  grpc_mdctx_unref(c->md_ctx);
   gpr_free(c);
 }
 
@@ -587,7 +587,7 @@ static void fake_oauth2_destroy(grpc_credentials *creds) {
   if (c->access_token_md != NULL) {
     grpc_mdelem_unref(c->access_token_md);
   }
-  grpc_mdctx_orphan(c->md_ctx);
+  grpc_mdctx_unref(c->md_ctx);
   gpr_free(c);
 }
 
@@ -897,7 +897,7 @@ static void iam_destroy(grpc_credentials *creds) {
   grpc_iam_credentials *c = (grpc_iam_credentials *)creds;
   grpc_mdelem_unref(c->token_md);
   grpc_mdelem_unref(c->authority_selector_md);
-  grpc_mdctx_orphan(c->md_ctx);
+  grpc_mdctx_unref(c->md_ctx);
   gpr_free(c);
 }
 
diff --git a/src/core/surface/channel.c b/src/core/surface/channel.c
index e308c60410..e38734c6a4 100644
--- a/src/core/surface/channel.c
+++ b/src/core/surface/channel.c
@@ -146,7 +146,7 @@ static void destroy_channel(void *p, int ok) {
   grpc_mdstr_unref(channel->grpc_message_string);
   grpc_mdstr_unref(channel->path_string);
   grpc_mdstr_unref(channel->authority_string);
-  grpc_mdctx_orphan(channel->metadata_context);
+  grpc_mdctx_unref(channel->metadata_context);
   gpr_free(channel);
 }
 
diff --git a/src/core/transport/chttp2_transport.c b/src/core/transport/chttp2_transport.c
index 6999d58102..5b2d0a5e5b 100644
--- a/src/core/transport/chttp2_transport.c
+++ b/src/core/transport/chttp2_transport.c
@@ -336,11 +336,9 @@ static void recv_data(void *tp, gpr_slice *slices, size_t nslices,
  * CONSTRUCTION/DESTRUCTION/REFCOUNTING
  */
 
-static void unref_transport(transport *t) {
+static void destruct_transport(transport *t) {
   size_t i;
 
-  if (!gpr_unref(&t->refs)) return;
-
   gpr_mu_lock(&t->mu);
 
   GPR_ASSERT(t->ep == NULL);
@@ -380,9 +378,16 @@ static void unref_transport(transport *t) {
 
   grpc_sopb_destroy(&t->nuke_later_sopb);
 
+  grpc_mdctx_unref(t->metadata_context);
+
   gpr_free(t);
 }
 
+static void unref_transport(transport *t) {
+  if (!gpr_unref(&t->refs)) return;
+  destruct_transport(t);
+}
+
 static void ref_transport(transport *t) { gpr_ref(&t->refs); }
 
 static void init_transport(transport *t, grpc_transport_setup_callback setup,
@@ -401,6 +406,7 @@ static void init_transport(transport *t, grpc_transport_setup_callback setup,
   gpr_ref_init(&t->refs, 2);
   gpr_mu_init(&t->mu);
   gpr_cv_init(&t->cv);
+  grpc_mdctx_ref(mdctx);
   t->metadata_context = mdctx;
   t->str_grpc_timeout =
       grpc_mdstr_from_string(t->metadata_context, "grpc-timeout");
diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c
index 3dc23e7de2..1c15716fad 100644
--- a/src/core/transport/metadata.c
+++ b/src/core/transport/metadata.c
@@ -79,7 +79,7 @@ typedef struct internal_metadata {
 
 struct grpc_mdctx {
   gpr_uint32 hash_seed;
-  int orphaned;
+  int refs;
 
   gpr_mu mu;
 
@@ -114,7 +114,7 @@ static void unlock(grpc_mdctx *ctx) {
      mdelems on every unlock (instead of the usual 'I'm too loaded' trigger
      case), since otherwise we can be stuck waiting for a garbage collection
      that will never happen. */
-  if (ctx->orphaned) {
+  if (ctx->refs == 0) {
     /* uncomment if you're having trouble diagnosing an mdelem leak to make
        things clearer (slows down destruction a lot, however) */
     /* gc_mdtab(ctx); */
@@ -139,7 +139,7 @@ static void ref_md(internal_metadata *md) {
 grpc_mdctx *grpc_mdctx_create_with_seed(gpr_uint32 seed) {
   grpc_mdctx *ctx = gpr_malloc(sizeof(grpc_mdctx));
 
-  ctx->orphaned = 0;
+  ctx->refs = 1;
   ctx->hash_seed = seed;
   gpr_mu_init(&ctx->mu);
   ctx->strtab = gpr_malloc(sizeof(internal_string *) * INITIAL_STRTAB_CAPACITY);
@@ -197,10 +197,17 @@ static void metadata_context_destroy(grpc_mdctx *ctx) {
   gpr_free(ctx);
 }
 
-void grpc_mdctx_orphan(grpc_mdctx *ctx) {
+void grpc_mdctx_ref(grpc_mdctx *ctx) {
   lock(ctx);
-  GPR_ASSERT(!ctx->orphaned);
-  ctx->orphaned = 1;
+  GPR_ASSERT(ctx->refs > 0);
+  ctx->refs++;
+  unlock(ctx);
+}
+
+void grpc_mdctx_unref(grpc_mdctx *ctx) {
+  lock(ctx);
+  GPR_ASSERT(ctx->refs > 0);
+  ctx->refs--;
   unlock(ctx);
 }
 
diff --git a/src/core/transport/metadata.h b/src/core/transport/metadata.h
index 430cae6847..7a56e34690 100644
--- a/src/core/transport/metadata.h
+++ b/src/core/transport/metadata.h
@@ -84,7 +84,8 @@ struct grpc_mdelem {
 /* Create/orphan a metadata context */
 grpc_mdctx *grpc_mdctx_create(void);
 grpc_mdctx *grpc_mdctx_create_with_seed(gpr_uint32 seed);
-void grpc_mdctx_orphan(grpc_mdctx *mdctx);
+void grpc_mdctx_ref(grpc_mdctx *mdctx);
+void grpc_mdctx_unref(grpc_mdctx *mdctx);
 
 /* Test only accessors to internal state - only for testing this code - do not
    rely on it outside of metadata_test.c */
diff --git a/test/core/channel/channel_stack_test.c b/test/core/channel/channel_stack_test.c
index 0345f99bde..59a4564220 100644
--- a/test/core/channel/channel_stack_test.c
+++ b/test/core/channel/channel_stack_test.c
@@ -128,7 +128,7 @@ static void test_create_channel_stack(void) {
   grpc_channel_stack_destroy(channel_stack);
   gpr_free(channel_stack);
 
-  grpc_mdctx_orphan(metadata_context);
+  grpc_mdctx_unref(metadata_context);
 }
 
 int main(int argc, char **argv) {
diff --git a/test/core/channel/metadata_buffer_test.c b/test/core/channel/metadata_buffer_test.c
index 22776f8ca1..ba8100b7d2 100644
--- a/test/core/channel/metadata_buffer_test.c
+++ b/test/core/channel/metadata_buffer_test.c
@@ -182,7 +182,7 @@ static void test_case(size_t key_prefix_len, size_t value_prefix_len,
   gpr_free(stk);
 
   grpc_metadata_buffer_destroy(&buffer, GRPC_OP_OK);
-  grpc_mdctx_orphan(mdctx);
+  grpc_mdctx_unref(mdctx);
 }
 
 int main(int argc, char **argv) {
diff --git a/test/core/security/credentials_test.c b/test/core/security/credentials_test.c
index 302869d70e..f911db6de1 100644
--- a/test/core/security/credentials_test.c
+++ b/test/core/security/credentials_test.c
@@ -138,7 +138,7 @@ static void test_oauth2_token_fetcher_creds_parsing_ok(void) {
   GPR_ASSERT(!strcmp(grpc_mdstr_as_c_string(token_elem->value),
                      "Bearer ya29.AHES6ZRN3-HlhAPya30GnW_bHSb_"));
   grpc_mdelem_unref(token_elem);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_oauth2_token_fetcher_creds_parsing_bad_http_status(void) {
@@ -150,7 +150,7 @@ static void test_oauth2_token_fetcher_creds_parsing_bad_http_status(void) {
   GPR_ASSERT(grpc_oauth2_token_fetcher_credentials_parse_server_response(
                  &response, ctx, &token_elem, &token_lifetime) ==
              GRPC_CREDENTIALS_ERROR);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_oauth2_token_fetcher_creds_parsing_empty_http_body(void) {
@@ -161,7 +161,7 @@ static void test_oauth2_token_fetcher_creds_parsing_empty_http_body(void) {
   GPR_ASSERT(grpc_oauth2_token_fetcher_credentials_parse_server_response(
                  &response, ctx, &token_elem, &token_lifetime) ==
              GRPC_CREDENTIALS_ERROR);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_oauth2_token_fetcher_creds_parsing_invalid_json(void) {
@@ -176,7 +176,7 @@ static void test_oauth2_token_fetcher_creds_parsing_invalid_json(void) {
   GPR_ASSERT(grpc_oauth2_token_fetcher_credentials_parse_server_response(
                  &response, ctx, &token_elem, &token_lifetime) ==
              GRPC_CREDENTIALS_ERROR);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_oauth2_token_fetcher_creds_parsing_missing_token(void) {
@@ -190,7 +190,7 @@ static void test_oauth2_token_fetcher_creds_parsing_missing_token(void) {
   GPR_ASSERT(grpc_oauth2_token_fetcher_credentials_parse_server_response(
                  &response, ctx, &token_elem, &token_lifetime) ==
              GRPC_CREDENTIALS_ERROR);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_oauth2_token_fetcher_creds_parsing_missing_token_type(void) {
@@ -205,7 +205,7 @@ static void test_oauth2_token_fetcher_creds_parsing_missing_token_type(void) {
   GPR_ASSERT(grpc_oauth2_token_fetcher_credentials_parse_server_response(
                  &response, ctx, &token_elem, &token_lifetime) ==
              GRPC_CREDENTIALS_ERROR);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_oauth2_token_fetcher_creds_parsing_missing_token_lifetime(
@@ -220,7 +220,7 @@ static void test_oauth2_token_fetcher_creds_parsing_missing_token_lifetime(
   GPR_ASSERT(grpc_oauth2_token_fetcher_credentials_parse_server_response(
                  &response, ctx, &token_elem, &token_lifetime) ==
              GRPC_CREDENTIALS_ERROR);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void check_metadata(expected_md *expected, grpc_mdelem **md_elems,
diff --git a/test/core/transport/chttp2/hpack_parser_test.c b/test/core/transport/chttp2/hpack_parser_test.c
index edab37b687..86c6bb1f56 100644
--- a/test/core/transport/chttp2/hpack_parser_test.c
+++ b/test/core/transport/chttp2/hpack_parser_test.c
@@ -214,7 +214,7 @@ static void test_vectors(grpc_slice_split_mode mode) {
               "set-cookie",
               "foo=ASDJKHQKBZXOQWEOPIUAXQWEOIU; max-age=3600; version=1", NULL);
   grpc_chttp2_hpack_parser_destroy(&parser);
-  grpc_mdctx_orphan(mdctx);
+  grpc_mdctx_unref(mdctx);
 }
 
 int main(int argc, char **argv) {
diff --git a/test/core/transport/chttp2/hpack_table_test.c b/test/core/transport/chttp2/hpack_table_test.c
index f3da9f0d49..d1e5f0829a 100644
--- a/test/core/transport/chttp2/hpack_table_test.c
+++ b/test/core/transport/chttp2/hpack_table_test.c
@@ -126,7 +126,7 @@ static void test_static_lookup(void) {
   assert_index(&tbl, 61, "www-authenticate", "");
 
   grpc_chttp2_hptbl_destroy(&tbl);
-  grpc_mdctx_orphan(mdctx);
+  grpc_mdctx_unref(mdctx);
 }
 
 static void test_many_additions(void) {
@@ -158,7 +158,7 @@ static void test_many_additions(void) {
   }
 
   grpc_chttp2_hptbl_destroy(&tbl);
-  grpc_mdctx_orphan(mdctx);
+  grpc_mdctx_unref(mdctx);
 }
 
 static grpc_chttp2_hptbl_find_result find_simple(grpc_chttp2_hptbl *tbl,
@@ -262,7 +262,7 @@ static void test_find(void) {
   GPR_ASSERT(r.has_value == 0);
 
   grpc_chttp2_hptbl_destroy(&tbl);
-  grpc_mdctx_orphan(mdctx);
+  grpc_mdctx_unref(mdctx);
 }
 
 int main(int argc, char **argv) {
diff --git a/test/core/transport/chttp2/stream_encoder_test.c b/test/core/transport/chttp2/stream_encoder_test.c
index 3013533f9b..5c7801079f 100644
--- a/test/core/transport/chttp2/stream_encoder_test.c
+++ b/test/core/transport/chttp2/stream_encoder_test.c
@@ -309,7 +309,7 @@ static void run_test(void (*test)(), const char *name) {
   grpc_sopb_init(&g_sopb);
   test();
   grpc_chttp2_hpack_compressor_destroy(&g_compressor);
-  grpc_mdctx_orphan(g_mdctx);
+  grpc_mdctx_unref(g_mdctx);
   grpc_sopb_destroy(&g_sopb);
 }
 
diff --git a/test/core/transport/metadata_test.c b/test/core/transport/metadata_test.c
index d003582a2f..f345cebdb6 100644
--- a/test/core/transport/metadata_test.c
+++ b/test/core/transport/metadata_test.c
@@ -52,7 +52,7 @@ static void test_no_op(void) {
   LOG_TEST();
 
   ctx = grpc_mdctx_create();
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_create_string(void) {
@@ -71,7 +71,7 @@ static void test_create_string(void) {
   GPR_ASSERT(gpr_slice_str_cmp(s3->slice, "very much not hello") == 0);
   grpc_mdstr_unref(s1);
   grpc_mdstr_unref(s2);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
   grpc_mdstr_unref(s3);
 }
 
@@ -95,7 +95,7 @@ static void test_create_metadata(void) {
   grpc_mdelem_unref(m1);
   grpc_mdelem_unref(m2);
   grpc_mdelem_unref(m3);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_create_many_ephemeral_metadata(void) {
@@ -116,7 +116,7 @@ static void test_create_many_ephemeral_metadata(void) {
   /* capacity should not grow */
   GPR_ASSERT(mdtab_capacity_before ==
              grpc_mdctx_get_mdtab_capacity_test_only(ctx));
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_create_many_persistant_metadata(void) {
@@ -145,7 +145,7 @@ static void test_create_many_persistant_metadata(void) {
   for (i = 0; i < MANY; i++) {
     grpc_mdelem_unref(created[i]);
   }
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 
   gpr_free(created);
 }
@@ -171,7 +171,7 @@ static void test_spin_creating_the_same_thing(void) {
   GPR_ASSERT(grpc_mdctx_get_mdtab_count_test_only(ctx) == 1);
   GPR_ASSERT(grpc_mdctx_get_mdtab_free_test_only(ctx) == 1);
 
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_things_stick_around(void) {
@@ -218,7 +218,7 @@ static void test_things_stick_around(void) {
     }
   }
 
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
   gpr_free(strs);
   gpr_free(shuf);
 }
@@ -245,7 +245,7 @@ static void test_slices_work(void) {
   gpr_slice_unref(slice);
   grpc_mdstr_unref(str);
 
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 static void test_base64_and_huffman_works(void) {
@@ -264,7 +264,7 @@ static void test_base64_and_huffman_works(void) {
 
   gpr_slice_unref(slice2);
   grpc_mdstr_unref(str);
-  grpc_mdctx_orphan(ctx);
+  grpc_mdctx_unref(ctx);
 }
 
 int main(int argc, char **argv) {
diff --git a/test/core/transport/transport_end2end_tests.c b/test/core/transport/transport_end2end_tests.c
index 6a0848fa97..6d13bf1f8c 100644
--- a/test/core/transport/transport_end2end_tests.c
+++ b/test/core/transport/transport_end2end_tests.c
@@ -927,7 +927,7 @@ void grpc_transport_end2end_tests(grpc_transport_test_config *config) {
     test_request_with_flow_ctl_cb(config, interesting_message_lengths[i]);
   }
 
-  grpc_mdctx_orphan(g_metadata_context);
+  grpc_mdctx_unref(g_metadata_context);
 
   gpr_log(GPR_INFO, "tests completed ok");
 }