Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash in tcp_proxy #4323

Merged
merged 9 commits into from
Sep 5, 2018
Merged

Fix crash in tcp_proxy #4323

merged 9 commits into from
Sep 5, 2018

Conversation

ggreenway
Copy link
Contributor

Closing the upstream connection is not safe from the Filter destructor,
because it triggers events back into the downstream connection, which
is partially destructed.

Ensure that the upstream connection is closed before the destructor is called.

Fixes #4310

Signed-off-by: Greg Greenway [email protected]

Risk Level: Medium
Testing: Unit and integration tests

@ggreenway
Fix crash in tcp_proxy.
b64f67c 
Closing the upstream connection is not safe from the Filter destructor,
because it triggers events back into the downstream connection, which
is partially destructed.

Ensure that the upstream connection is closed before the destructor is called.

Fixes envoyproxy#4310

Signed-off-by: Greg Greenway <[email protected]>
@ggreenway
Copy link
Contributor Author

Still working on this, and figuring out some tests, and expected behavior.

Another possible fix in tcp_proxy is to allow the destructor to close the upstream connection, but set a flag so that onUpstreamEvent() doesn't do anything.

@ggreenway ggreenway changed the title WIP: Fix crash in tcp_proxy Fix crash in tcp_proxy Sep 4, 2018
@ggreenway
Add test that covers new code
7e2361c 
Signed-off-by: Greg Greenway <[email protected]>
@ggreenway
Copy link
Contributor Author

@zuercher I think this is ready for review.

I wasn't able to test specifically for the case that I think caused the crash, because I think it requires the downstream connection to get closed, and the upstream connection to get established, in the same loop of the event loop. I couldn't think of any way to make that happen in an integration test.

@zuercher
Copy link
Member

zuercher commented Sep 4, 2018

re: testing -- I think there's a path where TransportSocket::doWrite is invoked via onWriteReady that might be testable? I think this must be the actual path for the crash anyway.

@ggreenway
fix build
756d2ac 
Signed-off-by: Greg Greenway <[email protected]>
@zuercher
Copy link
Member

zuercher commented Sep 5, 2018

Following up on my previous comment, I think this reproduces the crash in a test:

https://github.com/envoyproxy/envoy/compare/master...turbinelabs:stephan/tcp_proxy_crash_repro?expand=1

@zuercher
Copy link
Member

zuercher commented Sep 5, 2018

Besides getting the tests nailed down, this looks correct to me. Thanks for picking this up.

@ggreenway
fix tsan error in test (use after free)
f2b3fbf 
Signed-off-by: Greg Greenway <[email protected]>
@ggreenway
Revert tcp_proxy_integration_test fixes. These changes can't be made
5fab628 
until behavior changes to tcp_proxy are made.

Signed-off-by: Greg Greenway <[email protected]>
@zuercher zuercher self-assigned this Sep 5, 2018
@ggreenway
Copy link
Contributor Author

It looks like that test reproduces the failure, and ran against this PR it crashes when the test is destructing. I'm not sure how to finish the test so that it shuts down cleanly though. And I think a test like this will always be racy.

I think I'd rather not add a racy test if we can avoid it. What do you think about merging this as-is and iterating on testing more later?

zuercher
zuercher approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding more tests later works for me. Maybe file a ticket?

@ggreenway
Copy link
Contributor Author

Filed #4349

@ggreenway ggreenway merged commit e34dcd6 into envoyproxy:master Sep 5, 2018
@PiotrSikora
Copy link
Contributor

Thanks for fixing this, @ggreenway!

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
@PiotrSikora @istio-testing
Update Envoy SHA to latest with TCP proxy fixes. (envoyproxy#1964)
490d26f 
Pulling the following changes from github.com/envoyproxy/envoy:

f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345)
e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323)
ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326)
aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232)
5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250)
c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257)
752483e Fixing the fix (envoyproxy#4333)
83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338)
7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309)
69474b3 admin: order stats in clusters json admin (envoyproxy#4306)
2d155f9 ppc64le build (envoyproxy#4183)
07efc6d fix static initialization fiasco problem (envoyproxy#4314)
0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297)
1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead
d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328)
0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319)
cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335)
f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322)
e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329)
0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296)
00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321)
af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318)
3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311)
42f6048 Proto string issue fix (envoyproxy#4320)
9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256)
a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303)
1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307)
1212423 alts: add gRPC TSI socket (envoyproxy#4153)
f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300)
01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304)
1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308)
aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244)
89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269)
97eba59 build: bump googletest version. (envoyproxy#4293)
0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262)
9d094e5 Revert ac0bd74 (envoyproxy#4295)
ddb28a4 Add validation context provider (envoyproxy#4264)
3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986)
cf87d50 docs: update SNI FAQ. (envoyproxy#4285)
f952033 config: fix update empty stat for eds (envoyproxy#4276)
329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219)
68d20b4  thrift: refactor build files and imports (envoyproxy#4271)
5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144)
fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282)
53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927)
c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247)
cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188)
b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220)
0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252)
da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265)
9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253)
52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238)
f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239)
c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260)
35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255)
ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258)
b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248)
8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254)
28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233)
f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245)

Fixes istio/istio#8310 (once pulled into istio/istio).

Signed-off-by: Piotr Sikora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuercher zuercher zuercher approved these changes

Assignees

@zuercher zuercher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Short lived connections causing segmentation fault
3 participants
@ggreenway @zuercher @PiotrSikora