Add a null check for the endpoint on shutdown by kpayson64 · Pull Request #15727 · grpc/grpc

kpayson64 · 2018-06-12T20:25:57Z

We are seeing a null pointer exception internally on the call to grpc_endpoint_shutdown() a few lines below internally.

Hypothetical race condition:

1: A handshaker completes successfully
2: on_handshake_done is scheduled on the ExecCtx (with no error)
3: grpc_handshake_manager_shutdown() is invoked on some other thread.
4: The handshakers shutdown function shuts down the endpoint and sets it to null (see rpc-switch)
5: ExecCtx::Flush() gets invoked on the original thread, on_handshake_done is invoked with GRPC_ERROR_NONE and mgr->shutdown=true, and the endpoint has already been nulled out.

grpc-testing · 2018-06-12T20:29:24Z

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

libgrpc.so

     VM SIZE                                          FILE SIZE
 ++++++++++++++ GROWING                            ++++++++++++++
  +0.6%     +16 src/core/lib/channel/handshaker.cc     +16  +0.6%
      +1.9%     +16 call_next_handshaker_locked            +16  +1.9%

 -------------- SHRINKING                          --------------
  -0.0%     -16 [None]                                 -16  -0.0%

  [ = ]       0 TOTAL                                    0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-12T20:38:14Z

[trickle] No significant performance differences

grpc-testing · 2018-06-12T21:21:00Z

[microbenchmarks] No significant performance differences

markdroth

Good diagnosis of this problem! I think the fix is not quite right, though.

Is there some reasonable way to write a test to catch this?

markdroth · 2018-06-13T17:24:40Z

src/core/lib/channel/handshaker.cc

I think we want to reset error on the next line even if the endpoint is null. Otherwise, we will invoke mgr->on_handshake_done with no error, and the caller will expect the endpoint to be non-null, so we'd just be moving the crash to a different place.

grpc-testing · 2018-06-13T19:08:57Z

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

libgrpc.so

     VM SIZE                                          FILE SIZE
 ++++++++++++++ GROWING                            ++++++++++++++
  +0.6%     +16 src/core/lib/channel/handshaker.cc     +16  +0.6%
      +1.9%     +16 call_next_handshaker_locked            +16  +1.9%

 -------------- SHRINKING                          --------------
  -0.0%     -16 [None]                                 -16  -0.0%

  [ = ]       0 TOTAL                                    0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

kpayson64 · 2018-06-13T19:13:29Z

I don't see a way to force this failure case with any end-to-end tests because the core surface has no guarantees about when flush gets called.

I think the only way to test this would be set up some kind of mock no-op handshaker and a unit test for the handshake manager.

markdroth

Thanks for doing this, Ken!

I think the only way to test this would be set up some kind of mock no-op handshaker and a unit test for the handshake manager.

I agree. @yashykt, that's probably something to think about doing in the future, maybe as part of the C++-ification of the handshaker APIs.

markdroth · 2018-06-13T19:12:19Z

src/core/lib/channel/handshaker.cc

Please add a comment explaining the race condition that we're catching here (the case you describe in the PR description).

grpc-testing · 2018-06-13T19:16:07Z

[trickle] No significant performance differences

grpc-testing · 2018-06-13T19:26:32Z

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

libgrpc.so

     VM SIZE                                          FILE SIZE
 ++++++++++++++ GROWING                            ++++++++++++++
  +0.6%     +16 src/core/lib/channel/handshaker.cc     +16  +0.6%
      +1.9%     +16 call_next_handshaker_locked            +16  +1.9%

 -------------- SHRINKING                          --------------
  -0.0%     -16 [None]                                 -16  -0.0%

  [ = ]       0 TOTAL                                    0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-13T19:34:04Z

[trickle] No significant performance differences

grpc-testing · 2018-06-13T20:01:21Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-13T20:17:46Z

[microbenchmarks] Performance differences noted:
Benchmark                                                                                 cpu_time    real_time
----------------------------------------------------------------------------------------  ----------  -----------
BM_PumpStreamClientToServer<InProcess>/262144                                             +10%        +10%
BM_PumpStreamServerToClient<InProcess>/32768                                              +8%         +8%
BM_StreamingPingPong<InProcess, NoOpMutator, NoOpMutator>/262144/2                        +4%         +4%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144                      +6%         +6%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768                       +7%         +7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768                    +9%         +9%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/262144/2/1  +5%         +5%
BM_UnaryPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/262144                    -4%         -4%

grpc-testing · 2018-06-13T21:44:30Z

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

libgrpc.so

     VM SIZE                                          FILE SIZE
 ++++++++++++++ GROWING                            ++++++++++++++
  +0.6%     +16 src/core/lib/channel/handshaker.cc     +16  +0.6%
      +1.9%     +16 call_next_handshaker_locked            +16  +1.9%

 -------------- SHRINKING                          --------------
  -0.0%     -16 [None]                                 -16  -0.0%

  [ = ]       0 TOTAL                                    0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-13T21:51:46Z

[trickle] No significant performance differences

grpc-testing · 2018-06-13T22:36:03Z

[microbenchmarks] No significant performance differences

kpayson64 · 2018-06-14T00:06:54Z

#15693
#15751
#15752

kpayson64 requested a review from markdroth June 12, 2018 20:25

kpayson64 assigned markdroth Jun 12, 2018

markdroth reviewed Jun 13, 2018

View reviewed changes

markdroth approved these changes Jun 13, 2018

View reviewed changes

Add a null check for the endpoint on shutdown

0293ceb

kpayson64 force-pushed the null_endpoint branch from 4740c1d to 0293ceb Compare June 13, 2018 19:23

kpayson64 added the kokoro:force-run label Jun 13, 2018

kokoro-team removed the kokoro:force-run label Jun 13, 2018

kpayson64 merged commit 26287ea into grpc:master Jun 14, 2018

mehrdada mentioned this pull request Jun 15, 2018

Backport 15727 into v1.13.x (Add a null check for the endpoint on shutdown) #15781

Merged

srini100 mentioned this pull request Jun 28, 2018

Channel creation/destruction may leave dangling thread. #14338

Closed

kpayson64 added the release notes: no Indicates if PR should not be in release notes label Jul 23, 2018

lock bot locked as resolved and limited conversation to collaborators Oct 21, 2018

Conversation

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

Uh oh!

Uh oh!

grpc-testing commented Jun 12, 2018

Uh oh!

grpc-testing commented Jun 12, 2018

Uh oh!

grpc-testing commented Jun 12, 2018

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

kpayson64 commented Jun 13, 2018

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

kpayson64 Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

kpayson64 commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kpayson64 commented Jun 12, 2018 •

edited

Loading