Merge pull request #587 from ctiller/chex2

Fix use-after-free.

5 files changed, 28 insertions, 14 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 */