Don't segfault on header replay

grpc · Jan 26, 2018 · b15024d6a1537c69fc446601559a89dc8b84cf6f · b15024d
1 parent db68cb3
commit b15024d6a1537c69fc446601559a89dc8b84cf6f
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -555,6 +555,7 @@ endif()
 add_dependencies(buildtests_c public_headers_must_be_c89)
 add_dependencies(buildtests_c badreq_bad_client_test)
 add_dependencies(buildtests_c connection_prefix_bad_client_test)
+add_dependencies(buildtests_c duplicate_header_bad_client_test)
 add_dependencies(buildtests_c head_of_line_blocking_bad_client_test)
 add_dependencies(buildtests_c headers_bad_client_test)
 add_dependencies(buildtests_c initial_settings_frame_bad_client_test)
@@ -12892,6 +12893,37 @@ target_link_libraries(connection_prefix_bad_client_test
 endif (gRPC_BUILD_TESTS)
 if (gRPC_BUILD_TESTS)

+add_executable(duplicate_header_bad_client_test
+  test/core/bad_client/tests/duplicate_header.cc
+)
+
+
+target_include_directories(duplicate_header_bad_client_test
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include
+  PRIVATE ${BORINGSSL_ROOT_DIR}/include
+  PRIVATE ${PROTOBUF_ROOT_DIR}/src
+  PRIVATE ${BENCHMARK_ROOT_DIR}/include
+  PRIVATE ${ZLIB_ROOT_DIR}
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib
+  PRIVATE ${CARES_INCLUDE_DIR}
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/cares/cares
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include
+)
+
+target_link_libraries(duplicate_header_bad_client_test
+  ${_gRPC_SSL_LIBRARIES}
+  ${_gRPC_ALLTARGETS_LIBRARIES}
+  bad_client_test
+  grpc_test_util_unsecure
+  grpc_unsecure
+  gpr_test_util
+  gpr
+)
+
+endif (gRPC_BUILD_TESTS)
+if (gRPC_BUILD_TESTS)
+
 add_executable(head_of_line_blocking_bad_client_test
  test/core/bad_client/tests/head_of_line_blocking.cc
 )
diff --git a/Makefile b/Makefile
@@ -1223,6 +1223,7 @@ boringssl_tab_test: $(BINDIR)/$(CONFIG)/boringssl_tab_test
 boringssl_v3name_test: $(BINDIR)/$(CONFIG)/boringssl_v3name_test
 badreq_bad_client_test: $(BINDIR)/$(CONFIG)/badreq_bad_client_test
 connection_prefix_bad_client_test: $(BINDIR)/$(CONFIG)/connection_prefix_bad_client_test
+duplicate_header_bad_client_test: $(BINDIR)/$(CONFIG)/duplicate_header_bad_client_test
 head_of_line_blocking_bad_client_test: $(BINDIR)/$(CONFIG)/head_of_line_blocking_bad_client_test
 headers_bad_client_test: $(BINDIR)/$(CONFIG)/headers_bad_client_test
 initial_settings_frame_bad_client_test: $(BINDIR)/$(CONFIG)/initial_settings_frame_bad_client_test
@@ -1476,6 +1477,7 @@ buildtests_c: privatelibs_c \
  $(BINDIR)/$(CONFIG)/public_headers_must_be_c89 \
  $(BINDIR)/$(CONFIG)/badreq_bad_client_test \
  $(BINDIR)/$(CONFIG)/connection_prefix_bad_client_test \
+  $(BINDIR)/$(CONFIG)/duplicate_header_bad_client_test \
  $(BINDIR)/$(CONFIG)/head_of_line_blocking_bad_client_test \
  $(BINDIR)/$(CONFIG)/headers_bad_client_test \
  $(BINDIR)/$(CONFIG)/initial_settings_frame_bad_client_test \
@@ -2004,6 +2006,8 @@ test_c: buildtests_c
 	$(Q) $(BINDIR)/$(CONFIG)/badreq_bad_client_test || ( echo test badreq_bad_client_test failed ; exit 1 )
 	$(E) "[RUN]     Testing connection_prefix_bad_client_test"
 	$(Q) $(BINDIR)/$(CONFIG)/connection_prefix_bad_client_test || ( echo test connection_prefix_bad_client_test failed ; exit 1 )
+	$(E) "[RUN]     Testing duplicate_header_bad_client_test"
+	$(Q) $(BINDIR)/$(CONFIG)/duplicate_header_bad_client_test || ( echo test duplicate_header_bad_client_test failed ; exit 1 )
 	$(E) "[RUN]     Testing head_of_line_blocking_bad_client_test"
 	$(Q) $(BINDIR)/$(CONFIG)/head_of_line_blocking_bad_client_test || ( echo test head_of_line_blocking_bad_client_test failed ; exit 1 )
 	$(E) "[RUN]     Testing headers_bad_client_test"
@@ -18478,6 +18482,26 @@ ifneq ($(NO_DEPS),true)
 endif


+DUPLICATE_HEADER_BAD_CLIENT_TEST_SRC = \
+    test/core/bad_client/tests/duplicate_header.cc \
+
+DUPLICATE_HEADER_BAD_CLIENT_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(DUPLICATE_HEADER_BAD_CLIENT_TEST_SRC))))
+
+
+$(BINDIR)/$(CONFIG)/duplicate_header_bad_client_test: $(DUPLICATE_HEADER_BAD_CLIENT_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libbad_client_test.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util_unsecure.a $(LIBDIR)/$(CONFIG)/libgrpc_unsecure.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
+	$(E) "[LD]      Linking $@"
+	$(Q) mkdir -p `dirname $@`
+	$(Q) $(LD) $(LDFLAGS) $(DUPLICATE_HEADER_BAD_CLIENT_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libbad_client_test.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util_unsecure.a $(LIBDIR)/$(CONFIG)/libgrpc_unsecure.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) -o $(BINDIR)/$(CONFIG)/duplicate_header_bad_client_test
+
+$(OBJDIR)/$(CONFIG)/test/core/bad_client/tests/duplicate_header.o:  $(LIBDIR)/$(CONFIG)/libbad_client_test.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util_unsecure.a $(LIBDIR)/$(CONFIG)/libgrpc_unsecure.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
+
+deps_duplicate_header_bad_client_test: $(DUPLICATE_HEADER_BAD_CLIENT_TEST_OBJS:.o=.dep)
+
+ifneq ($(NO_DEPS),true)
+-include $(DUPLICATE_HEADER_BAD_CLIENT_TEST_OBJS:.o=.dep)
+endif
+
+
 HEAD_OF_LINE_BLOCKING_BAD_CLIENT_TEST_SRC = \
    test/core/bad_client/tests/head_of_line_blocking.cc \

diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc
@@ -1101,6 +1101,7 @@ static grpc_stream_compression_algorithm decode_stream_compression(
 static void publish_app_metadata(grpc_call* call, grpc_metadata_batch* b,
                                 int is_trailing) {
  if (b->list.count == 0) return;
+  if (is_trailing && call->buffered_metadata[1] == nullptr) return;
  GPR_TIMER_BEGIN("publish_app_metadata", 0);
  grpc_metadata_array* dest;
  grpc_metadata* mdusr;
diff --git a/test/core/bad_client/gen_build_yaml.py b/test/core/bad_client/gen_build_yaml.py
@@ -27,6 +27,7 @@
 BAD_CLIENT_TESTS = {
    'badreq': default_test_options,
    'connection_prefix': default_test_options._replace(cpu_cost=0.2),
+    'duplicate_header': default_test_options,
    'headers': default_test_options._replace(cpu_cost=0.2),
    'initial_settings_frame': default_test_options._replace(cpu_cost=0.2),
    'head_of_line_blocking': default_test_options,
diff --git a/test/core/bad_client/generate_tests.bzl b/test/core/bad_client/generate_tests.bzl
@@ -25,6 +25,7 @@ def test_options():
 BAD_CLIENT_TESTS = {
    'badreq': test_options(),
    'connection_prefix': test_options(),
+    'duplicate_header': test_options(),
    'headers': test_options(),
    'initial_settings_frame': test_options(),
    'head_of_line_blocking': test_options(),
diff --git a/test/core/bad_client/tests/duplicate_header.cc b/test/core/bad_client/tests/duplicate_header.cc
@@ -0,0 +1,134 @@
+/*
+ *
+ * Copyright 2018 gRPC authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+#include "test/core/bad_client/bad_client.h"
+
+#include <string.h>
+
+#include <grpc/grpc.h>
+
+#include "src/core/lib/surface/server.h"
+#include "test/core/end2end/cq_verifier.h"
+
+#define PFX_STR                      \
+  "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" \
+  "\x00\x00\x00\x04\x00\x00\x00\x00\x00" /* settings frame */
+
+#define HEADER_STR                                                         \
+  "\x00\x00\xc9\x01\x04\x00\x00\x00\x01" /* headers: generated from        \
+                                            simple_request.headers in this \
+                                            directory */                   \
+  "\x10\x05:path\x08/foo/bar"                                              \
+  "\x10\x07:scheme\x04http"                                                \
+  "\x10\x07:method\x04POST"                                                \
+  "\x10\x0a:authority\x09localhost"                                        \
+  "\x10\x0c"                                                               \
+  "content-type\x10"                                                       \
+  "application/grpc"                                                       \
+  "\x10\x14grpc-accept-encoding\x15"                                       \
+  "deflate,identity,gzip"                                                  \
+  "\x10\x02te\x08trailers"                                                 \
+  "\x10\x0auser-agent\"bad-client grpc-c/0.12.0.0 (linux)"
+
+#define PAYLOAD_STR                      \
+  "\x00\x00\x20\x00\x00\x00\x00\x00\x01" \
+  "\x00\x00\x00\x00"
+
+static void* tag(intptr_t t) { return (void*)t; }
+
+static void verifier(grpc_server* server, grpc_completion_queue* cq,
+                     void* registered_method) {
+  grpc_call_error error;
+  grpc_call* s;
+  grpc_call_details call_details;
+  grpc_byte_buffer* request_payload_recv = nullptr;
+  grpc_op* op;
+  grpc_op ops[6];
+  cq_verifier* cqv = cq_verifier_create(cq);
+  grpc_metadata_array request_metadata_recv;
+  int was_cancelled = 2;
+
+  grpc_call_details_init(&call_details);
+  grpc_metadata_array_init(&request_metadata_recv);
+
+  error = grpc_server_request_call(server, &s, &call_details,
+                                   &request_metadata_recv, cq, cq, tag(101));
+  GPR_ASSERT(GRPC_CALL_OK == error);
+  CQ_EXPECT_COMPLETION(cqv, tag(101), 1);
+  cq_verify(cqv);
+
+  GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.host, "localhost"));
+  GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo/bar"));
+
+  memset(ops, 0, sizeof(ops));
+  op = ops;
+  op->op = GRPC_OP_SEND_INITIAL_METADATA;
+  op->data.send_initial_metadata.count = 0;
+  op->flags = 0;
+  op->reserved = nullptr;
+  op++;
+  op->op = GRPC_OP_RECV_MESSAGE;
+  op->data.recv_message.recv_message = &request_payload_recv;
+  op->flags = 0;
+  op->reserved = nullptr;
+  op++;
+  error = grpc_call_start_batch(s, ops, (size_t)(op - ops), tag(102), nullptr);
+  GPR_ASSERT(GRPC_CALL_OK == error);
+
+  CQ_EXPECT_COMPLETION(cqv, tag(102), 1);
+  cq_verify(cqv);
+
+  memset(ops, 0, sizeof(ops));
+  op = ops;
+  op->op = GRPC_OP_RECV_CLOSE_ON_SERVER;
+  op->data.recv_close_on_server.cancelled = &was_cancelled;
+  op->flags = 0;
+  op->reserved = nullptr;
+  op++;
+  op->op = GRPC_OP_SEND_STATUS_FROM_SERVER;
+  op->data.send_status_from_server.trailing_metadata_count = 0;
+  op->data.send_status_from_server.status = GRPC_STATUS_UNIMPLEMENTED;
+  grpc_slice status_details = grpc_slice_from_static_string("xyz");
+  op->data.send_status_from_server.status_details = &status_details;
+  op->flags = 0;
+  op->reserved = nullptr;
+  op++;
+  error = grpc_call_start_batch(s, ops, (size_t)(op - ops), tag(103), nullptr);
+  GPR_ASSERT(GRPC_CALL_OK == error);
+
+  CQ_EXPECT_COMPLETION(cqv, tag(103), 1);
+
+  grpc_metadata_array_destroy(&request_metadata_recv);
+  grpc_call_details_destroy(&call_details);
+  grpc_call_unref(s);
+  cq_verifier_destroy(cqv);
+}
+
+int main(int argc, char** argv) {
+  grpc_test_init(argc, argv);
+  grpc_init();
+
+  /* Verify that sending multiple headers doesn't segfault */
+  GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr,
+                           PFX_STR HEADER_STR HEADER_STR PAYLOAD_STR, 0);
+  GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr,
+                           PFX_STR HEADER_STR HEADER_STR HEADER_STR PAYLOAD_STR,
+                           0);
+  grpc_shutdown();
+  return 0;
+}
diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json
@@ -4865,6 +4865,24 @@
    "third_party": false, 
    "type": "target"
  }, 
+  {
+    "deps": [
+      "bad_client_test", 
+      "gpr", 
+      "gpr_test_util", 
+      "grpc_test_util_unsecure", 
+      "grpc_unsecure"
+    ], 
+    "headers": [], 
+    "is_filegroup": false, 
+    "language": "c", 
+    "name": "duplicate_header_bad_client_test", 
+    "src": [
+      "test/core/bad_client/tests/duplicate_header.cc"
+    ], 
+    "third_party": false, 
+    "type": "target"
+  }, 
  {
    "deps": [
      "bad_client_test", 
diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json
@@ -4554,6 +4554,32 @@
    ], 
    "uses_polling": true
  }, 
+  {
+    "args": [], 
+    "benchmark": false, 
+    "ci_platforms": [
+      "linux", 
+      "mac", 
+      "posix", 
+      "windows"
+    ], 
+    "cpu_cost": 1.0, 
+    "exclude_configs": [], 
+    "exclude_iomgrs": [
+      "uv"
+    ], 
+    "flaky": false, 
+    "gtest": false, 
+    "language": "c", 
+    "name": "duplicate_header_bad_client_test", 
+    "platforms": [
+      "linux", 
+      "mac", 
+      "posix", 
+      "windows"
+    ], 
+    "uses_polling": true
+  }, 
  {
    "args": [], 
    "benchmark": false,