Don't segfault on header replay #14191

kpayson64 · 2018-01-26T00:54:13Z

Once approved will backport fix to 1.8/1.9

grpc-testing · 2018-01-26T00:57:10Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.0%     +24 [None]                                                                                  +528  +0.0%
  +0.4%     +64 src/core/lib/surface/call.cc                                                             +64  +0.4%
      [NEW]    +506 set_encodings_accepted_by_peer(grpc_call*, grpc_mdelem, unsigned int*, bool) [clone     +506  [NEW]
      [NEW]    +266 publish_app_metadata(grpc_call*, grpc_metadata_batch*, int) [clone .isra.5] [clone .    +266  [NEW]
      +1.2%     +16 finish_batch_step                                                                        +16  +1.2%
      +1.0%     +15 receiving_initial_metadata_ready                                                         +15  +1.0%
      +0.6%      +2 [Unmapped]                                                                                +2  +0.6%

  +0.0%     +88 TOTAL                                                                                   +592  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

yang-g · 2018-01-26T01:00:02Z

src/core/lib/surface/call.cc

  GPR_TIMER_BEGIN("publish_app_metadata", 0);
  grpc_metadata_array* dest;
  grpc_metadata* mdusr;
  dest = call->buffered_metadata[is_trailing];
+  if (dest == NULL) gpr_log(GPR_ERROR, "dest %p", dest);


What is the point of this log line?

Oops, good catch

yang-g · 2018-01-26T01:00:40Z

test/core/bad_client/tests/duplicate_header.cc

@@ -0,0 +1,131 @@
+/*
+ *
+ * Copyright 2017 gRPC authors.


yang-g · 2018-01-26T01:02:31Z

test/core/bad_client/tests/duplicate_header.cc

+
+  /* Verify that sending multiple headers doesn't segfault */
+  GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr,
+                           PFX_STR HEADER_STR HEADER_STR PAYLOAD_STR, 0);


Do we want to test other cases?
For example:

PFX_STR HEADER_STR PAYLOAD_STR(no EOS) HEADER_STR

Duplicate HEADERS does not contain reserved keys

It is also interesting to see 3 HEADERS will cause what... if that is not too much...

That's not a bad idea.

Also, if at all possible, it'd be nice to inject that into the fuzzer's corpus, so it can further try and discover bugs around this.

I've added the 3 leading headers to the test. I can add more variants into the fuzzer's corpus.

(FWIW, PFX_STR HEADER_STR PAYLOAD_STR(no EOS) HEADER_STR doesnt segfault, but cq->next returns an unsuccessful event)

That can be further investigated without blocking this fix.

grpc-testing · 2018-01-26T01:08:26Z

[trickle] No significant performance differences

nicolasnoble · 2018-01-26T01:21:18Z

I think you wanted to open that PR against 1.8, and not master.

nicolasnoble · 2018-01-26T01:22:25Z

Right and I can't just change the base, it will otherwise try to bring all of master into 1.8.

yang-g · 2018-01-26T01:27:07Z

LGTM if the 3 headers case pass

grpc-testing · 2018-01-26T01:48:43Z

[microbenchmarks] No significant performance differences

kpayson64 · 2018-01-26T18:34:45Z

I've addressed comments and rebased to 1.8.x

grpc-testing · 2018-01-26T18:36:13Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                       FILE SIZE
 ++++++++++++++ GROWING                                                                         ++++++++++++++
  +0.1%     +16 src/core/lib/surface/call.cc                                                        +16  +0.1%
      +6.8%     +16 publish_app_metadata(grpc_call*, grpc_metadata_batch*, int) [clone .isra.6]         +16  +6.8%

 -+-+-+-+-+-+-+ MIXED                                                                           +-+-+-+-+-+-+-
  -0.0%     -16 [None]                                                                              +24  +0.0%

  [ = ]       0 TOTAL                                                                               +40  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

yang-g

Thanks for the fix.

grpc-testing · 2018-01-26T18:48:33Z

[trickle] No significant performance differences

grpc-testing · 2018-01-26T19:27:49Z

[microbenchmarks] No significant performance differences

ctiller · 2018-01-26T20:19:58Z

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;


It'd be better to find a fix in chttp2: fixes at this layer are more likely to break with efforts to remove this layer entirely in the c++ stack.

Right, this is a temporary bandaid. We want to actually return a 400 from within chttp2, but "stop the bleeding first".

kpayson64 assigned nicolasnoble Jan 26, 2018

kpayson64 requested a review from nicolasnoble January 26, 2018 00:54

yang-g reviewed Jan 26, 2018

View reviewed changes

nicolasnoble changed the base branch from master to v1.8.x January 26, 2018 01:21

nicolasnoble changed the base branch from v1.8.x to master January 26, 2018 01:21

nicolasnoble added the priority/P0/RELEASE BLOCKER label Jan 26, 2018

Don't segfault on header replay

b15024d

kpayson64 force-pushed the bug_fix branch from e61ccac to b15024d Compare January 26, 2018 18:33

kpayson64 changed the base branch from master to v1.8.x January 26, 2018 18:33

yang-g approved these changes Jan 26, 2018

View reviewed changes

kpayson64 mentioned this pull request Jan 26, 2018

Don't segfault on header replay #14199

Merged

mehrdada added priority/P1 and removed priority/P0/RELEASE BLOCKER labels Jan 26, 2018

ctiller reviewed Jan 26, 2018

View reviewed changes

nicolasnoble merged commit 2d52c78 into grpc:v1.8.x Jan 26, 2018

kpayson64 mentioned this pull request Jan 29, 2018

Return HTTP 400 on receiving multiple header frames #14223

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't segfault on header replay #14191

Don't segfault on header replay #14191

kpayson64 commented Jan 26, 2018 •

edited

grpc-testing commented Jan 26, 2018

yang-g Jan 26, 2018

kpayson64 Jan 26, 2018

yang-g Jan 26, 2018

kpayson64 Jan 26, 2018

yang-g Jan 26, 2018

yang-g Jan 26, 2018

nicolasnoble Jan 26, 2018

nicolasnoble Jan 26, 2018

kpayson64 Jan 26, 2018

yang-g Jan 26, 2018

grpc-testing commented Jan 26, 2018

nicolasnoble commented Jan 26, 2018

nicolasnoble commented Jan 26, 2018

yang-g commented Jan 26, 2018

grpc-testing commented Jan 26, 2018

kpayson64 commented Jan 26, 2018

grpc-testing commented Jan 26, 2018

yang-g left a comment

grpc-testing commented Jan 26, 2018

grpc-testing commented Jan 26, 2018

ctiller Jan 26, 2018

nicolasnoble Jan 26, 2018

Don't segfault on header replay #14191

Don't segfault on header replay #14191

Conversation

kpayson64 commented Jan 26, 2018 • edited

grpc-testing commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grpc-testing commented Jan 26, 2018

nicolasnoble commented Jan 26, 2018

nicolasnoble commented Jan 26, 2018

yang-g commented Jan 26, 2018

grpc-testing commented Jan 26, 2018

kpayson64 commented Jan 26, 2018

grpc-testing commented Jan 26, 2018

yang-g left a comment

Choose a reason for hiding this comment

grpc-testing commented Jan 26, 2018

grpc-testing commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpayson64 commented Jan 26, 2018 •

edited