Don't segfault on header replay by kpayson64 · Pull Request #14191 · grpc/grpc

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

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

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

test/core/bad_client/tests/duplicate_header.cc

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

 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

Conversation

kpayson64 commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grpc-testing commented Jan 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jan 26, 2018

Uh oh!

nicolasnoble commented Jan 26, 2018

Uh oh!

nicolasnoble commented Jan 26, 2018

Uh oh!

yang-g commented Jan 26, 2018

Uh oh!

grpc-testing commented Jan 26, 2018

Uh oh!

kpayson64 commented Jan 26, 2018

Uh oh!

grpc-testing commented Jan 26, 2018

Uh oh!

yang-g left a comment

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jan 26, 2018

Uh oh!

grpc-testing commented Jan 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kpayson64 commented Jan 26, 2018 •

edited

Loading