Skip to content

http: fixing a fuzz flake by setting details on connection teardown #12737

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

Merged
snowp merged 2 commits into from
Aug 21, 2020
Merged

http: fixing a fuzz flake by setting details on connection teardown #12737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad6f9d1
http: fixing a fuzz flake by making sure streams killed during shutdo…
alyssawilk Aug 19, 2020
a657c64
fixing an unexpecfied spelling error
alyssawilk Aug 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/envoy/stream_info/stream_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ struct ResponseCodeDetailValues {
const std::string FilterChainNotFound = "filter_chain_not_found";
// The client disconnected unexpectedly.
const std::string DownstreamRemoteDisconnect = "downstream_remote_disconnect";
// The client connection was locally closed for an unspecified reason.
const std::string DownstreamLocalDisconnect = "downstream_local_disconnect";
// The max connection duration was exceeded.
const std::string DurationTimeout = "duration_timeout";
// The response was generated by the admin filter.
const std::string AdminFilterResponse = "admin_filter_response";
// The original stream was replaced with an internal redirect.
Expand Down
52 changes: 25 additions & 27 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ ConnectionManagerImpl::~ConnectionManagerImpl() {

void ConnectionManagerImpl::checkForDeferredClose() {
if (drain_state_ == DrainState::Closing && streams_.empty() && !codec_->wantsToWrite()) {
doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay, absl::nullopt);
doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay, absl::nullopt,
StreamInfo::ResponseCodeDetails::get().DownstreamLocalDisconnect);
}
}

Expand Down Expand Up @@ -263,15 +264,14 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod

void ConnectionManagerImpl::handleCodecError(absl::string_view error) {
ENVOY_CONN_LOG(debug, "dispatch error: {}", read_callbacks_->connection(), error);
read_callbacks_->connection().streamInfo().setResponseCodeDetails(
absl::StrCat("codec error: ", error));
read_callbacks_->connection().streamInfo().setResponseFlag(
StreamInfo::ResponseFlag::DownstreamProtocolError);

// HTTP/1.1 codec has already sent a 400 response if possible. HTTP/2 codec has already sent
// GOAWAY.
doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay,
StreamInfo::ResponseFlag::DownstreamProtocolError);
StreamInfo::ResponseFlag::DownstreamProtocolError,
absl::StrCat("codec error: ", error));
}

void ConnectionManagerImpl::createCodec(Buffer::Instance& data) {
Expand Down Expand Up @@ -354,8 +354,8 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() {
return Network::FilterStatus::StopIteration;
}

void ConnectionManagerImpl::resetAllStreams(
absl::optional<StreamInfo::ResponseFlag> response_flag) {
void ConnectionManagerImpl::resetAllStreams(absl::optional<StreamInfo::ResponseFlag> response_flag,
absl::string_view details) {
while (!streams_.empty()) {
// Mimic a downstream reset in this case. We must also remove callbacks here. Though we are
// about to close the connection and will disable further reads, it is possible that flushing
Expand All @@ -367,23 +367,16 @@ void ConnectionManagerImpl::resetAllStreams(
// codec but there are no easy answers and this seems simpler.
auto& stream = *streams_.front();
stream.response_encoder_->getStream().removeCallbacks(stream);
stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view());
if (!stream.response_encoder_->getStream().responseDetails().empty()) {
stream.filter_manager_.streamInfo().setResponseCodeDetails(
stream.response_encoder_->getStream().responseDetails());
} else if (!details.empty()) {
stream.filter_manager_.streamInfo().setResponseCodeDetails(details);
}

@asraa asraa Aug 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

qq -- this fixes these three cases, but there's no "catch all" for if the response details are still empty. This in the hopes that the ASSERT will reveal anything missing, is that right?

@alyssawilk alyssawilk Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, the assert should take care of it.
The only place we set empty details is the idle timeout, which should only fire if there are no streams to terminate. If others add other empty strings it's annoying and will trigger a debug assert for "you are doing the wrong thing" but given it's just missing debug info, not mission critical, I think that's fine.

if (response_flag.has_value()) {
// This code duplicates some of the logic in
// onResetStream(). There seems to be no easy way to force
// onResetStream to do the right thing within its current API.
// Encoding DownstreamProtocolError as reason==LocalReset does
// not work because local reset is generated in other places.
// Encoding it in the string_view argument would lead to a hack
// of the form: if parameter is nonempty, use that; else if the
// codec details are nonempty, use those. This hack does not
// seem better than the code duplication, so punt for now.
stream.filter_manager_.streamInfo().setResponseFlag(response_flag.value());
if (*response_flag == StreamInfo::ResponseFlag::DownstreamProtocolError) {
stream.filter_manager_.streamInfo().setResponseCodeDetails(
stream.response_encoder_->getStream().responseDetails());
}
}
stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view());
}
}

Expand All @@ -398,6 +391,10 @@ void ConnectionManagerImpl::onEvent(Network::ConnectionEvent event) {
remote_close_ = true;
stats_.named_.downstream_cx_destroy_remote_.inc();
}
absl::string_view details =
event == Network::ConnectionEvent::RemoteClose
? StreamInfo::ResponseCodeDetails::get().DownstreamRemoteDisconnect
: StreamInfo::ResponseCodeDetails::get().DownstreamLocalDisconnect;
// TODO(mattklein123): It is technically possible that something outside of the filter causes
// a local connection close, so we still guard against that here. A better solution would be to
// have some type of "pre-close" callback that we could hook for cleanup that would get called
Expand All @@ -407,13 +404,13 @@ void ConnectionManagerImpl::onEvent(Network::ConnectionEvent event) {
// NOTE: In the case where a local close comes from outside the filter, this will cause any
// stream closures to increment remote close stats. We should do better here in the future,
// via the pre-close callback mentioned above.
doConnectionClose(absl::nullopt, absl::nullopt);
doConnectionClose(absl::nullopt, absl::nullopt, details);
}
}

void ConnectionManagerImpl::doConnectionClose(
absl::optional<Network::ConnectionCloseType> close_type,
absl::optional<StreamInfo::ResponseFlag> response_flag) {
absl::optional<StreamInfo::ResponseFlag> response_flag, absl::string_view details) {
if (connection_idle_timer_) {
connection_idle_timer_->disableTimer();
connection_idle_timer_.reset();
Expand Down Expand Up @@ -445,7 +442,7 @@ void ConnectionManagerImpl::doConnectionClose(
// Note that resetAllStreams() does not actually write anything to the wire. It just resets
// all upstream streams and their filter stacks. Thus, there are no issues around recursive
// entry.
resetAllStreams(response_flag);
resetAllStreams(response_flag, details);
}

if (close_type.has_value()) {
Expand All @@ -464,7 +461,7 @@ void ConnectionManagerImpl::onIdleTimeout() {
if (!codec_) {
// No need to delay close after flushing since an idle timeout has already fired. Attempt to
// write out buffered data one last time and issue a local close if successful.
doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt);
doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt, "");
} else if (drain_state_ == DrainState::NotDraining) {
startDrainSequence();
}
Expand All @@ -475,7 +472,8 @@ void ConnectionManagerImpl::onConnectionDurationTimeout() {
stats_.named_.downstream_cx_max_duration_reached_.inc();
if (!codec_) {
// Attempt to write out buffered data one last time and issue a local close if successful.
doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt);
doConnectionClose(Network::ConnectionCloseType::FlushWrite, absl::nullopt,
StreamInfo::ResponseCodeDetails::get().DurationTimeout);
} else if (drain_state_ == DrainState::NotDraining) {
startDrainSequence();
}
Expand Down Expand Up @@ -633,7 +631,6 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() {
filter_manager_.streamInfo().setResponseCodeDetails(
StreamInfo::ResponseCodeDetails::get().DownstreamRemoteDisconnect);
}

if (connection_manager_.codec_->protocol() < Protocol::Http2) {
// For HTTP/2 there are still some reset cases where details are not set.
// For HTTP/1 there shouldn't be any. Regression-proof this.
Expand Down Expand Up @@ -2176,7 +2173,6 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl:
// If we need to differentiate we need to do it inside the codec. Can start with this.
ENVOY_STREAM_LOG(debug, "stream reset", *this);
connection_manager_.stats_.named_.downstream_rq_rx_reset_.inc();
connection_manager_.doDeferredStreamDestroy(*this);

// If the codec sets its responseDetails(), impute a
// DownstreamProtocolError and propagate the details upwards.
Expand All @@ -2185,6 +2181,8 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason, absl:
filter_manager_.streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError);
filter_manager_.streamInfo().setResponseCodeDetails(encoder_details);
}

connection_manager_.doDeferredStreamDestroy(*this);
}

void ConnectionManagerImpl::ActiveStream::onAboveWriteBufferHighWatermark() {
Expand Down
6 changes: 4 additions & 2 deletions source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,15 +1007,17 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
*/
void doEndStream(ActiveStream& stream);

void resetAllStreams(absl::optional<StreamInfo::ResponseFlag> response_flag);
void resetAllStreams(absl::optional<StreamInfo::ResponseFlag> response_flag,
absl::string_view details);
void onIdleTimeout();
void onConnectionDurationTimeout();
void onDrainTimeout();
void startDrainSequence();
Tracing::HttpTracer& tracer() { return *config_.tracer(); }
void handleCodecError(absl::string_view error);
void doConnectionClose(absl::optional<Network::ConnectionCloseType> close_type,
absl::optional<StreamInfo::ResponseFlag> response_flag);
absl::optional<StreamInfo::ResponseFlag> response_flag,
absl::string_view details);

enum class DrainState { NotDraining, Draining, Closing };

Expand Down
14 changes: 11 additions & 3 deletions test/common/http/conn_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3665,7 +3665,9 @@ TEST_F(HttpConnectionManagerImplTest, TestDownstreamProtocolErrorAfterHeadersAcc

// Verify that FrameFloodException causes connection to be closed abortively.
TEST_F(HttpConnectionManagerImplTest, FrameFloodError) {
InSequence s;
std::shared_ptr<AccessLog::MockInstance> log_handler =
std::make_shared<NiceMock<AccessLog::MockInstance>>();
access_logs_ = {log_handler};
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
Expand All @@ -3680,14 +3682,20 @@ TEST_F(HttpConnectionManagerImplTest, FrameFloodError) {
EXPECT_CALL(filter_callbacks_.connection_,
close(Network::ConnectionCloseType::FlushWriteAndDelay));

EXPECT_CALL(*log_handler, log(_, _, _, _))
.WillOnce(Invoke([](const HeaderMap*, const HeaderMap*, const HeaderMap*,
const StreamInfo::StreamInfo& stream_info) {
ASSERT_TRUE(stream_info.responseCodeDetails().has_value());
EXPECT_EQ("codec error: too many outbound frames.",
stream_info.responseCodeDetails().value());
}));
// Kick off the incoming data.
Buffer::OwnedImpl fake_input("1234");
EXPECT_LOG_NOT_CONTAINS("warning", "downstream HTTP flood",
conn_manager_->onData(fake_input, false));

EXPECT_TRUE(filter_callbacks_.connection_.streamInfo().hasResponseFlag(
StreamInfo::ResponseFlag::DownstreamProtocolError));
EXPECT_EQ("codec error: too many outbound frames.",
filter_callbacks_.connection_.streamInfo().responseCodeDetails().value());
}

TEST_F(HttpConnectionManagerImplTest, IdleTimeoutNoCodec) {
Expand Down