network: catch SIGPIPEs via SO_NOSIGPIPE on Darwin by rebello95 · Pull Request #12039 · envoyproxy/envoy

rebello95 · 2020-07-10T23:32:13Z

With Envoy Mobile, we have started seeing some SIGPIPE fatal crashes in our iOS application. This is a bit strange considering Envoy attempts to suppress SIGPIPE errors. However, this is done at the global process-wide level, meaning something else could be re-setting it.

It's worth noting that:

We only started seeing this after updating Envoy Mobile's Envoy submodule with this diff. Binary searches unfortunately were not of much help since this is a pretty rare crash that we've only seen reported from production users' devices (no reproducible steps).
The above diff also includes some socket changes (and a few others), which could potentially be related to why we just now started seeing this issue (cc @florincoras in case you have ideas!).
The earliest build where we started seeing this issue was built with Xcode 11.5 rather than Xcode 11.3.1. Though unlikely, it is possible that this affected the effectiveness of Envoy's ignoring of SIGPIPE if something changed there.

This fix takes advantage of Apple's SO_NOSIGPIPE setting on a per-socket level to catch the SIGPIPEs that we're now seeing. It's being set on the upstream and listener socket creation codepaths in line with Envoy's existing setup to catch all SIGPIPEs.

SO_NOSIGPIPE is an option that prevents SIGPIPE from being raised when a write fails on a socket to which there is no reader; instead, the write to the socket returns with the error EPIPE when there is no reader.

Risk Level: Low, Envoy was already attempting to suppress SIGPIPEs
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Michael Rebello me@michaelrebello.com
Co-authored-by: Jose Nino jnino@lyft.com

rebello95 · 2020-07-10T23:33:51Z

cc @junr03 @goaway @florincoras

florincoras · 2020-07-11T00:27:12Z

@rebello95 interesting. As far as I know, we are not trying to re-set SIGPIPE (and a quick code search seems to confirm that). So, I guess that SIGPIPE suppression never worked but something else changed, maybe event scheduling/handling, and this is now surfacing.

rebello95 · 2020-07-11T15:28:38Z

Definitely possible. Unfortunately I haven't been able to confirm the exact root cause since it hasn't been reproducible.

Signed-off-by: Michael Rebello <me@michaelrebello.com>

junr03 · 2020-07-13T18:24:50Z

@rebello95 I think the master merge is missing DCO

rebello95 · 2020-07-13T18:25:35Z

@junr03 yep, rebased and fixed DCO just now

junr03 · 2020-07-13T18:25:40Z

@ggreenway I was wondering if you could take a look at this. I feel this is up your alley.

Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 · 2020-07-13T20:20:59Z

I think CI failures are unrelated - updated with master 🤷‍♂️

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123

LGTM with a few small comments, thanks.

/wait

source/common/network/socket_option_factory.h

mattklein123 · 2020-07-14T16:30:27Z

source/common/upstream/upstream_impl.cc

+  Network::Socket::appendOptions(cluster_options,
+                                 Network::SocketOptionFactory::buildSocketSigpipeOptions());


If we have this here and on the listener side, do we still need the process-wide signal() call that attempts to ignore SIGPIPE? I suppose that would be a riskier change, but can you add a TODO above that call to consider removing it and maybe also add some comments here and on the listener side of why we set this?

I think we'll always need the process-wide signal() call since this additional socket option only applies on certain OSes, as gated by #ifdef SO_NOSIGPIPE in this PR, so it probably doesn't make sense to have a todo. I updated with additional documentation as requested.

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123

LGTM please check format.

/wait

Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 · 2020-07-14T17:33:01Z

@mattklein123 fixed

rebello95 · 2020-07-14T17:41:21Z

And thank you for the reviews!

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123 · 2020-07-14T19:19:57Z

CI failures look legit.

/wait

Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 · 2020-07-16T00:34:30Z

Fixed most of the tests, but still working with @junr03 to resolve the rest

Signed-off-by: Jose Nino <jnino@lyft.com>

Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 · 2020-07-17T04:16:36Z

Should be good now!

mattklein123

Thanks!

Bumping upstream to include iOS fixes for SIGPIPE error handling: envoyproxy/envoy#12039 Signed-off-by: Michael Rebello <me@michaelrebello.com>

Signed-off-by: Michael Rebello <me@michaelrebello.com> Co-authored-by: Jose Nino <jnino@lyft.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Signed-off-by: Michael Rebello <me@michaelrebello.com> Co-authored-by: Jose Nino <jnino@lyft.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

Bumping upstream to include iOS fixes for SIGPIPE error handling: #12039 Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

rebello95 changed the title ~~fix sigpipe issue on darwin~~ network: catch SIGPIPEs via SO_NOSIGPIPE on Darwin Jul 10, 2020

fix sigpipe issue on darwin

5acbda3

Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 force-pushed the sigpipe-fix branch from e3a7662 to 5acbda3 Compare July 13, 2020 18:25

junr03 assigned ggreenway Jul 13, 2020

Merge remote-tracking branch 'origin/master' into sigpipe-fix

ea431c5

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123 self-assigned this Jul 14, 2020

Merge remote-tracking branch 'origin/master' into sigpipe-fix

b7682ff

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123 requested changes Jul 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 14, 2020

CR

e76a0ed

Signed-off-by: Michael Rebello <me@michaelrebello.com>

repokitteh-read-only bot removed the waiting label Jul 14, 2020

rebello95 requested a review from mattklein123 July 14, 2020 16:50

mattklein123 reviewed Jul 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 14, 2020

spellcheck

e1ea93e

Signed-off-by: Michael Rebello <me@michaelrebello.com>

repokitteh-read-only bot removed the waiting label Jul 14, 2020

try

ae9bb7f

Signed-off-by: Michael Rebello <me@michaelrebello.com>

repokitteh-read-only bot added the waiting label Jul 14, 2020

fix some tests

02c78c0

Signed-off-by: Michael Rebello <me@michaelrebello.com>

repokitteh-read-only bot removed the waiting label Jul 15, 2020

rebello95 added 3 commits July 15, 2020 16:26

green locally

b5a4081

Signed-off-by: Michael Rebello <me@michaelrebello.com>

formatting

41a32cb

Signed-off-by: Michael Rebello <me@michaelrebello.com>

Merge remote-tracking branch 'origin/master' into sigpipe-fix

1c67b62

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123 added the waiting label Jul 16, 2020

fmt is messed up

218658c

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 17, 2020

format

b0e1520

Signed-off-by: Michael Rebello <me@michaelrebello.com>

mattklein123 approved these changes Jul 17, 2020

View reviewed changes

mattklein123 merged commit 7d8b9ac into envoyproxy:master Jul 17, 2020

rebello95 deleted the sigpipe-fix branch July 17, 2020 16:32

rebello95 added a commit to envoyproxy/envoy-mobile that referenced this pull request Jul 17, 2020

upstream: include fix for SIGPIPE handling

45fdd27

Bumping upstream to include iOS fixes for SIGPIPE error handling: envoyproxy/envoy#12039 Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 mentioned this pull request Jul 17, 2020

upstream: include fix for SIGPIPE handling envoyproxy/envoy-mobile#965

Merged

		Network::Socket::appendOptions(cluster_options,
		Network::SocketOptionFactory::buildSocketSigpipeOptions());

Conversation

rebello95 commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebello95 commented Jul 10, 2020

Uh oh!

florincoras commented Jul 11, 2020

Uh oh!

rebello95 commented Jul 11, 2020

Uh oh!

junr03 commented Jul 13, 2020

Uh oh!

rebello95 commented Jul 13, 2020

Uh oh!

junr03 commented Jul 13, 2020

Uh oh!

rebello95 commented Jul 13, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

rebello95 Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

rebello95 commented Jul 14, 2020

Uh oh!

rebello95 commented Jul 14, 2020

Uh oh!

mattklein123 commented Jul 14, 2020

Uh oh!

rebello95 commented Jul 16, 2020

Uh oh!

rebello95 commented Jul 17, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rebello95 commented Jul 10, 2020 •

edited

Loading