Fix bug whereby receiving a trailer after a stream close could trigger an assert

6 files changed, 8 insertions, 49 deletions

diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
index 9b4c4f4220..d46656ce81 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
@@ -1102,7 +1102,7 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt,
 
   if (grpc_http_trace) {
     char *str = grpc_transport_stream_op_string(op);
-    gpr_log(GPR_DEBUG, "perform_stream_op: %s", str);
+    gpr_log(GPR_DEBUG, "perform_stream_op[s=%p/%d]: %s", s, s->id, str);
     gpr_free(str);
   }
 
@@ -1171,7 +1171,7 @@ static void perform_transport_op_locked(grpc_exec_ctx *exec_ctx,
   if (op->send_goaway) {
     t->sent_goaway = 1;
     grpc_chttp2_goaway_append(
-        t->last_incoming_stream_id,
+        t->last_new_stream_id,
         (uint32_t)grpc_chttp2_grpc_status_to_http2_error(op->goaway_status),
         gpr_slice_ref(*op->goaway_message), &t->qbuf);
     close_transport = grpc_chttp2_stream_map_size(&t->stream_map) == 0
diff --git a/src/core/ext/transport/chttp2/transport/frame_settings.c b/src/core/ext/transport/chttp2/transport/frame_settings.c
index fc0383d2e0..92022f90c9 100644
--- a/src/core/ext/transport/chttp2/transport/frame_settings.c
+++ b/src/core/ext/transport/chttp2/transport/frame_settings.c
@@ -224,7 +224,7 @@ grpc_error *grpc_chttp2_settings_parser_parse(grpc_exec_ctx *exec_ctx, void *p,
                 break;
               case GRPC_CHTTP2_DISCONNECT_ON_INVALID_VALUE:
                 grpc_chttp2_goaway_append(
-                    t->last_incoming_stream_id, sp->error_value,
+                    t->last_new_stream_id, sp->error_value,
                     gpr_slice_from_static_string("HTTP2 settings error"),
                     &t->qbuf);
                 gpr_asprintf(&msg, "invalid value %u passed for %s",
diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h
index 6b3e2edd54..3263c99bde 100644
--- a/src/core/ext/transport/chttp2/transport/internal.h
+++ b/src/core/ext/transport/chttp2/transport/internal.h
@@ -267,8 +267,8 @@ struct grpc_chttp2_transport {
   /** how far to lookahead in a stream? */
   uint32_t stream_lookahead;
 
-  /** last received stream id */
-  uint32_t last_incoming_stream_id;
+  /** last new stream id */
+  uint32_t last_new_stream_id;
 
   /** pings awaiting responses */
   grpc_chttp2_outstanding_ping pings;
diff --git a/src/core/ext/transport/chttp2/transport/parsing.c b/src/core/ext/transport/chttp2/transport/parsing.c
index aa36f90cae..41bf10b50f 100644
--- a/src/core/ext/transport/chttp2/transport/parsing.c
+++ b/src/core/ext/transport/chttp2/transport/parsing.c
@@ -199,10 +199,6 @@ grpc_error *grpc_chttp2_perform_read(grpc_exec_ctx *exec_ctx,
       if (err != GRPC_ERROR_NONE) {
         return err;
       }
-      if (t->incoming_stream_id != 0 &&
-          t->incoming_stream_id > t->last_incoming_stream_id) {
-        t->last_incoming_stream_id = t->incoming_stream_id;
-      }
       if (t->incoming_frame_size == 0) {
         err = parse_frame_slice(exec_ctx, t, gpr_empty_slice(), 1);
         if (err != GRPC_ERROR_NONE) {
@@ -578,12 +574,12 @@ static grpc_error *init_header_frame_parser(grpc_exec_ctx *exec_ctx,
                 "ignoring new grpc_chttp2_stream creation on client");
       }
       return init_skip_frame_parser(exec_ctx, t, 1);
-    } else if (t->last_incoming_stream_id > t->incoming_stream_id) {
+    } else if (t->last_new_stream_id >= t->incoming_stream_id) {
       gpr_log(GPR_ERROR,
               "ignoring out of order new grpc_chttp2_stream request on server; "
               "last grpc_chttp2_stream "
               "id=%d, new grpc_chttp2_stream id=%d",
-              t->last_incoming_stream_id, t->incoming_stream_id);
+              t->last_new_stream_id, t->incoming_stream_id);
       return init_skip_frame_parser(exec_ctx, t, 1);
     } else if ((t->incoming_stream_id & 1) == 0) {
       gpr_log(GPR_ERROR,
@@ -591,6 +587,7 @@ static grpc_error *init_header_frame_parser(grpc_exec_ctx *exec_ctx,
               t->incoming_stream_id);
       return init_skip_frame_parser(exec_ctx, t, 1);
     }
+    t->last_new_stream_id = t->incoming_stream_id;
     s = t->incoming_stream =
         grpc_chttp2_parsing_accept_stream(exec_ctx, t, t->incoming_stream_id);
     if (s == NULL) {
diff --git a/src/core/ext/transport/chttp2/transport/stream_map.c b/src/core/ext/transport/chttp2/transport/stream_map.c
index bd07412274..59b3a14e0a 100644
--- a/src/core/ext/transport/chttp2/transport/stream_map.c
+++ b/src/core/ext/transport/chttp2/transport/stream_map.c
@@ -97,40 +97,6 @@ void grpc_chttp2_stream_map_add(grpc_chttp2_stream_map *map, uint32_t key,
   map->count = count + 1;
 }
 
-void grpc_chttp2_stream_map_move_into(grpc_chttp2_stream_map *src,
-                                      grpc_chttp2_stream_map *dst) {
-  /* if src is empty we dont need to do anything */
-  if (src->count == src->free) {
-    return;
-  }
-  /* if dst is empty we simply need to swap */
-  if (dst->count == dst->free) {
-    GPR_SWAP(grpc_chttp2_stream_map, *src, *dst);
-    return;
-  }
-  /* the first element of src must be greater than the last of dst...
-   * however the maps may need compacting for this property to hold */
-  if (src->keys[0] <= dst->keys[dst->count - 1]) {
-    src->count = compact(src->keys, src->values, src->count);
-    src->free = 0;
-    dst->count = compact(dst->keys, dst->values, dst->count);
-    dst->free = 0;
-  }
-  GPR_ASSERT(src->keys[0] > dst->keys[dst->count - 1]);
-  /* if dst doesn't have capacity, resize */
-  if (dst->count + src->count > dst->capacity) {
-    dst->capacity = GPR_MAX(dst->capacity * 3 / 2, dst->count + src->count);
-    dst->keys = gpr_realloc(dst->keys, dst->capacity * sizeof(uint32_t));
-    dst->values = gpr_realloc(dst->values, dst->capacity * sizeof(void *));
-  }
-  memcpy(dst->keys + dst->count, src->keys, src->count * sizeof(uint32_t));
-  memcpy(dst->values + dst->count, src->values, src->count * sizeof(void *));
-  dst->count += src->count;
-  dst->free += src->free;
-  src->count = 0;
-  src->free = 0;
-}
-
 static void **find(grpc_chttp2_stream_map *map, uint32_t key) {
   size_t min_idx = 0;
   size_t max_idx = map->count;
diff --git a/src/core/ext/transport/chttp2/transport/stream_map.h b/src/core/ext/transport/chttp2/transport/stream_map.h
index b1d59ca6a3..e76312dd1a 100644
--- a/src/core/ext/transport/chttp2/transport/stream_map.h
+++ b/src/core/ext/transport/chttp2/transport/stream_map.h
@@ -65,10 +65,6 @@ void grpc_chttp2_stream_map_add(grpc_chttp2_stream_map *map, uint32_t key,
    or NULL otherwise */
 void *grpc_chttp2_stream_map_delete(grpc_chttp2_stream_map *map, uint32_t key);
 
-/* Move all elements of src into dst */
-void grpc_chttp2_stream_map_move_into(grpc_chttp2_stream_map *src,
-                                      grpc_chttp2_stream_map *dst);
-
 /* Return an existing key, or NULL if it does not exist */
 void *grpc_chttp2_stream_map_find(grpc_chttp2_stream_map *map, uint32_t key);