From 4ee1a627230c8564dfdab5247e8703e0eb10d5c8 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 29 Apr 2016 16:47:27 -0700 Subject: [PATCH] Stress test fixes - properly fail a Read() on a stream if we fail to parse a protobuf - fix an ordering problem with the chttp2 transport global lock, whereby a sequence of two operations could be swapped - this resulted in slices being returned to the upper layers in the wrong order, corrupting data --- include/grpc++/impl/codegen/call.h | 7 +++---- .../chttp2/transport/chttp2_transport.c | 17 ++++++++++++----- .../ext/transport/chttp2/transport/internal.h | 3 ++- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index aea1a6acec7..d081b7d9c59 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -281,10 +281,9 @@ class CallOpRecvMessage { if (message_ == nullptr) return; if (recv_buf_) { if (*status) { - got_message = true; - *status = SerializationTraits::Deserialize(recv_buf_, message_, - max_message_size) - .ok(); + got_message = *status = SerializationTraits::Deserialize( + recv_buf_, message_, max_message_size) + .ok(); } else { got_message = false; g_core_codegen_interface->grpc_byte_buffer_destroy(recv_buf_); diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index fcf2abfe66e..8c8593748dd 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -629,9 +629,10 @@ static void finish_global_actions(grpc_exec_ctx *exec_ctx, check_read_ops(exec_ctx, &t->global); gpr_mu_lock(&t->executor.mu); - if (t->executor.pending_actions != NULL) { - hdr = t->executor.pending_actions; - t->executor.pending_actions = NULL; + if (t->executor.pending_actions_head != NULL) { + hdr = t->executor.pending_actions_head; + t->executor.pending_actions_head = t->executor.pending_actions_tail = + NULL; gpr_mu_unlock(&t->executor.mu); while (hdr != NULL) { hdr->action(exec_ctx, t, hdr->stream, hdr->arg); @@ -686,8 +687,14 @@ void grpc_chttp2_run_with_global_lock(grpc_exec_ctx *exec_ctx, gpr_free(hdr); continue; } - hdr->next = t->executor.pending_actions; - t->executor.pending_actions = hdr; + hdr->next = NULL; + if (t->executor.pending_actions_head != NULL) { + t->executor.pending_actions_tail = + t->executor.pending_actions_tail->next = hdr; + } else { + t->executor.pending_actions_tail = t->executor.pending_actions_head = + hdr; + } REF_TRANSPORT(t, "pending_action"); gpr_mu_unlock(&t->executor.mu); } diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 7a8084641d7..a269338b494 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -321,7 +321,8 @@ struct grpc_chttp2_transport { /** is a thread currently parsing */ bool parsing_active; - grpc_chttp2_executor_action_header *pending_actions; + grpc_chttp2_executor_action_header *pending_actions_head; + grpc_chttp2_executor_action_header *pending_actions_tail; } executor; /** is the transport destroying itself? */