http: Fix ASSERT failure and infinite loop when attempting to unset readDisable state on a closed connection. #9509

antoniovicente · 2019-12-27T18:24:43Z

Description: Handle calls to ConnectionImpl::readDisable on Closed connections more gracefully. There are several places under source/common, including ClientConnectionImpl::onMessageComplete, where Connection::readDisable(false) is called on a loop to clear the readDisable state as part of the process of preparing upstream connections for reuse. It is possible to end up in an infinite loop if proxying an HTTP/1 response is framed by connection close, the call to onData triggers readDisable(true), and there is a pending PostIoAction::Close action. The closeSocket call on the upstreamConnection triggers the call to ClientConnectionImpl::onMessageComplete
Risk Level: Low
Testing: unit and integration tests
Docs Changes: n/a
Release Notes: n/a
Fixes #9508

…able state on a closed connection. Signed-off-by: Antonio Vicente <avd@google.com>

source/common/network/connection_impl.cc

…d_disabled_close Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123

Awesome, thanks for debugging and fixing this. In general this makes sense to me. Optimally I would like @alyssawilk to review, but if this needs to merge urgently we can do that and she can post-review. Also need to check format.

/wait

antoniovicente · 2020-01-02T15:25:18Z

Awesome, thanks for debugging and fixing this. In general this makes sense to me. Optimally I would like @alyssawilk to review, but if this needs to merge urgently we can do that and she can post-review. Also need to check format.

I think it would be fine to wait until @alyssawilk is back from the holidays

…d_disabled_close Signed-off-by: Antonio Vicente <avd@google.com>

…rectly. Signed-off-by: Antonio Vicente <avd@google.com>

alyssawilk

Good find - thanks for tackling it!

source/common/network/connection_impl.cc

alyssawilk · 2020-01-06T15:45:38Z

test/integration/integration_test.cc

@@ -221,6 +221,10 @@ TEST_P(IntegrationTest, RouterUpstreamDisconnectBeforeResponseComplete) {
  testRouterUpstreamDisconnectBeforeResponseComplete();
 }

+TEST_P(IntegrationTest, ResponseFramedByConnectionCloseWithReadLimits) {
+  testResponseFramedByConnectionCloseWithReadLimits();


If this is only used in one place please just put the code inline - it's not totally obvious but I'm trying to push for inline code where things are not reused.

alyssawilk · 2020-01-06T15:47:05Z

test/integration/http_integration.cc

+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+  waitForNextUpstreamRequest();
+  // Disable chunking to trigger framing by connection close.
+  upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {":no-chunks", "1"}},


This is just run for HTTP/1.1 right?
ostensibly the no chunk header is never supposed to be sent on the wire - I think if you don't send a content length and send HTTP/1.0 (see existing example tests) you can force frame by close that way. Mind trying it out?

I'm having some trouble finding examples under test/integration that encode an HTTP/1.0 response. Do you have some links handy?

As far as I can tell ":no-chunks" is the way to tell StreamEncoderImpl::encodeHeaders in source/common/http/http1/codec_impl.cc to not chunk a response without a content-length.

grep around for config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost); ?

They all use sendRawHttpAndWaitForResponse but given this is a header only request and you're waiting for connection close I think that should be fine.

I'm fairly sure that the :no-chunk does not end up on the wire. One thing that I wanted to verify in some way is that the Envoy does see an upstream response framed by connection close. I'm not sure how to accomplish that, since I think envoy will add chunk encoding to the response it sends downstream.

Let's talk briefly offline.

… readEnabled is called. Fix typo "chunking" -> "chunk encoding" Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

source/common/http/http1/codec_impl.cc

source/common/network/connection_impl.cc

mattklein123

LGTM, thanks for the fixes here. Will defer to @alyssawilk for further review.

source/common/http/http1/codec_impl.cc

mattklein123 · 2020-01-07T16:57:07Z

test/integration/integration_test.cc

+  upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {":no-chunks", "1"}},
+                                   false);


If you wanted to avoid the :no-chunks business, you could potentially just use a raw TCP fake upstream connection and manually send an HTTP/1.0 response (you could I suppose also build HTTP/1.0 handling into the fake upstream but that sounds like a lot more work), which IIRC Envoy should handle correctly with close framing. I don't feel strongly about it but I know @alyssawilk commented on this.

I take Antonio's point that :no-chunks doesn't reach the wire - I'm (warily) fine with it.

has alyssawilk seal of approval :-P

lambdai · 2020-03-09T17:48:25Z

/backport

PiotrSikora · 2020-03-26T07:06:43Z

@lambdai could you create the PR against release/v1.12? Thanks!

…isable state on a closed connection (#190) This is a port of envoyproxy#9509 Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

…eadDisable state on a closed connection. (envoyproxy#9509) Description: Handle calls to ConnectionImpl::readDisable on Closed connections more gracefully. There are several places under source/common, including ClientConnectionImpl::onMessageComplete, where Connection::readDisable(false) is called on a loop to clear the readDisable state as part of the process of preparing upstream connections for reuse. It is possible to end up in an infinite loop if proxying an HTTP/1 response is framed by connection close, the call to onData triggers readDisable(true), and there is a pending PostIoAction::Close action. The closeSocket call on the upstreamConnection triggers the call to ClientConnectionImpl::onMessageComplete Risk Level: medium Testing: unit and integration tests Docs Changes: n/a Release Notes: n/a Fixes envoyproxy#9508 Signed-off-by: Antonio Vicente <avd@google.com> Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Fix ASSERT failure and infinite loop when attempting to unset readDis…

1991223

…able state on a closed connection. Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz reviewed Dec 27, 2019

View reviewed changes

source/common/network/connection_impl.cc Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into fix_assert_on_rea…

dbe0c0c

…d_disabled_close Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123 self-assigned this Dec 29, 2019

mattklein123 previously requested changes Dec 29, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 29, 2019

Merge remote-tracking branch 'upstream/master' into fix_assert_on_rea…

15aa91b

…d_disabled_close Signed-off-by: Antonio Vicente <avd@google.com>

repokitteh-read-only bot removed the waiting label Jan 2, 2020

Use high-watermark of 1 in test now that low-watermark of 0 works cor…

37b2a9d

…rectly. Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123 assigned alyssawilk Jan 3, 2020

mattklein123 added the waiting:any label Jan 3, 2020

alyssawilk reviewed Jan 6, 2020

View reviewed changes

repokitteh-read-only bot removed the waiting:any label Jan 6, 2020

mattklein123 added the waiting label Jan 6, 2020

antoniovicente added 2 commits January 6, 2020 18:23

Restore ASSERT in readDisable and ASSERT that connection is Open when…

08e35d1

… readEnabled is called. Fix typo "chunking" -> "chunk encoding" Signed-off-by: Antonio Vicente <avd@google.com>

Move test implementation inline

e9b5914

Signed-off-by: Antonio Vicente <avd@google.com>

repokitteh-read-only bot removed the waiting label Jan 6, 2020

alyssawilk reviewed Jan 7, 2020

View reviewed changes

source/common/http/http1/codec_impl.cc Show resolved Hide resolved

source/common/network/connection_impl.cc Show resolved Hide resolved

mattklein123 reviewed Jan 7, 2020

View reviewed changes

alyssawilk approved these changes Jan 7, 2020

View reviewed changes

alyssawilk merged commit 9de6027 into envoyproxy:master Jan 7, 2020

antoniovicente mentioned this pull request Jan 21, 2020

hystrix: emit ":no-chunks" pseudo-header only in HTTP/1.x streams. #9737

Merged

This was referenced Feb 26, 2020

Spurious segmentation fault in flushMetricsToSinks -> MetricSnapshotImpl #10179

Open

Segmentation faults in Lua filter (LuaThreadLocal) #10241

Closed

repokitteh-read-only bot added the backport/review Request to backport to stable releases label Mar 9, 2020

PiotrSikora added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Mar 26, 2020

lambdai mentioned this pull request Apr 16, 2020

release-v1.12: back port #9509 http: Fix ASSERT failure and infinite loop when attempting to unset readDisable state on a closed connection #10802

Closed

asraa mentioned this pull request Nov 25, 2020

[security] Error handling policy and macro usage clarification #14190

Closed

4 tasks

vkarunx mentioned this pull request Jul 11, 2022

Envoy: idle CPU usage high namespacelabs/foundation#762

Closed

		upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {":no-chunks", "1"}},
		false);

http: Fix ASSERT failure and infinite loop when attempting to unset readDisable state on a closed connection. #9509

http: Fix ASSERT failure and infinite loop when attempting to unset readDisable state on a closed connection. #9509

Uh oh!

Conversation

antoniovicente commented Dec 27, 2019

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

antoniovicente commented Jan 2, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai commented Mar 9, 2020

Uh oh!

PiotrSikora commented Mar 26, 2020

Uh oh!

Uh oh!