Skip to content

upstream: Null-deref on TCP health checker if setsockopt fails #6793

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
mattklein123 merged 13 commits into from
May 10, 2019
Merged

upstream: Null-deref on TCP health checker if setsockopt fails #6793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ab89ac2
upstream: Fix null-deref in TcpHealthChecker if conn LocalClosed
May 2, 2019
609cfec
upstream: ConnectionLocalFailure unit test to simulate ossfuzz 11100
May 2, 2019
413fe8a
REBASE Honor clang-format
May 2, 2019
14c343c
REBASE Use expect_reset_ instead of stealing client to follow other c…
May 7, 2019
f3fda0e
Merge remote-tracking branch 'upstream/master' into fix-ossfuzz-11100
May 7, 2019
1b95a69
REBASE Reorder expect_reset_: if an inline reset happens, failure
May 7, 2019
d0a53bf
REBASE Add a test of timing out twice -> Unhealthy
May 7, 2019
9acd444
REBASE honor fix-format (sorry, i need to finish updating my build env)
May 7, 2019
260074f
Add unit test that 1 timeout causes unhealthy
May 9, 2019
57d549c
Add one-timeout tests for HTTP and gRPC
May 10, 2019
b77ef7d
REBASE Clarify that for TCP, we're handling expected closes (not Envo…
May 10, 2019
2eaee67
Convert TCP test configs to v2yaml, remove unused parseHealthCheckFro…
May 10, 2019
5721aca
Merge remote-tracking branch 'upstream/master' into fix-ossfuzz-11100
May 10, 2019
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
12 changes: 8 additions & 4 deletions source/common/upstream/health_checker_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ TcpHealthCheckerImpl::TcpActiveHealthCheckSession::~TcpActiveHealthCheckSession(

void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onDeferredDelete() {
if (client_) {
expect_close_ = true;
client_->close(Network::ConnectionCloseType::NoFlush);
}
}
Expand All @@ -371,6 +372,7 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onData(Buffer::Instance&
data.drain(data.length());
handleSuccess(false);
if (!parent_.reuse_connection_) {
expect_close_ = true;
client_->close(Network::ConnectionCloseType::NoFlush);
}
} else {
Expand All @@ -379,12 +381,11 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onData(Buffer::Instance&
}

void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onEvent(Network::ConnectionEvent event) {
if (event == Network::ConnectionEvent::RemoteClose) {
handleFailure(envoy::data::core::v2alpha::HealthCheckFailureType::NETWORK);
}

if (event == Network::ConnectionEvent::RemoteClose ||
event == Network::ConnectionEvent::LocalClose) {
if (!expect_close_) {
handleFailure(envoy::data::core::v2alpha::HealthCheckFailureType::NETWORK);
}
parent_.dispatcher_.deferredDelete(std::move(client_));
}

Expand All @@ -403,6 +404,7 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onEvent(Network::Connect
// TODO(mattklein123): In the case that a user configured bytes to write, they will not be
// be written, since we currently have no way to know if the bytes actually get written via
// the connection interface. We might want to figure out how to handle this better later.
expect_close_ = true;
client_->close(Network::ConnectionCloseType::NoFlush);
handleSuccess(false);
}
Expand All @@ -416,6 +418,7 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onInterval() {
client_->addConnectionCallbacks(*session_callbacks_);
client_->addReadFilter(session_callbacks_);

expect_close_ = false;
client_->connect();
client_->noDelay(true);
}
Expand All @@ -431,6 +434,7 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onInterval() {
}

void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onTimeout() {
expect_close_ = true;
host_->setActiveHealthFailureType(Host::ActiveHealthFailureType::TIMEOUT);
client_->close(Network::ConnectionCloseType::NoFlush);
}
Expand Down
3 changes: 3 additions & 0 deletions source/common/upstream/health_checker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ class TcpHealthCheckerImpl : public HealthCheckerImplBase {
TcpHealthCheckerImpl& parent_;
Network::ClientConnectionPtr client_;
std::shared_ptr<TcpSessionCallbacks> session_callbacks_;
// If true, stream close was initiated by us, not e.g. remote close or TCP reset.
// In this case healthcheck status already reported, only state cleanup required.
bool expect_close_{};
};

typedef std::unique_ptr<TcpActiveHealthCheckSession> TcpActiveHealthCheckSessionPtr;
Expand Down
Loading