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 2 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
28 changes: 18 additions & 10 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 @@ -347,7 +347,9 @@ TcpHealthCheckerImpl::TcpHealthCheckerImpl(const Cluster& cluster,

TcpHealthCheckerImpl::TcpActiveHealthCheckSession::~TcpActiveHealthCheckSession() {
if (client_) {
client_->close(Network::ConnectionCloseType::NoFlush);
Network::ClientConnectionPtr clientToDestroy(std::move(client_));
Comment thread
andrewjjenkins marked this conversation as resolved.
Outdated
clientToDestroy->close(Network::ConnectionCloseType::NoFlush);
parent_.dispatcher_.deferredDelete(std::move(clientToDestroy));
}
}

Expand All @@ -358,21 +360,23 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onData(Buffer::Instance&
data.drain(data.length());
handleSuccess(false);
if (!parent_.reuse_connection_) {
client_->close(Network::ConnectionCloseType::NoFlush);
Network::ClientConnectionPtr clientToDestroy(std::move(client_));
clientToDestroy->close(Network::ConnectionCloseType::NoFlush);
parent_.dispatcher_.deferredDelete(std::move(clientToDestroy));
}
} else {
host_->setActiveHealthFailureType(Host::ActiveHealthFailureType::UNHEALTHY);
}
}

void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onEvent(Network::ConnectionEvent event) {
if (event == Network::ConnectionEvent::RemoteClose) {
// If !client_, then we are already handling a failure/teardown

@andrewjjenkins andrewjjenkins May 2, 2019

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.

Key is this. I'm using !client_ as a signal that this is an internally-generated RemoteClose or LocalClose. I need some way to distinguish so I know that I need to call handleFailure().

(Other ways to distinguish internally-generated from externally-generated would work too)

@mattklein123 mattklein123 May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The other way to do this would be to do what the other health checkers are doing which is effectively the expect_reset boolean. I'm fine either way.

@andrewjjenkins andrewjjenkins May 7, 2019

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.

I updated to follow the expect_reset_ boolean pattern for consistency with the other health checkers, thanks for the suggestion.

if (client_ &&
(event == Network::ConnectionEvent::RemoteClose ||
event == Network::ConnectionEvent::LocalClose)) {
Network::ClientConnectionPtr clientToDestroy(std::move(client_));
handleFailure(envoy::data::core::v2alpha::HealthCheckFailureType::NETWORK);
}

if (event == Network::ConnectionEvent::RemoteClose ||
event == Network::ConnectionEvent::LocalClose) {
parent_.dispatcher_.deferredDelete(std::move(client_));
parent_.dispatcher_.deferredDelete(std::move(clientToDestroy));
}

if (event == Network::ConnectionEvent::Connected && parent_.receive_bytes_.empty()) {
Expand All @@ -390,7 +394,9 @@ 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.
client_->close(Network::ConnectionCloseType::NoFlush);
Network::ClientConnectionPtr clientToDestroy(std::move(client_));
clientToDestroy->close(Network::ConnectionCloseType::NoFlush);
parent_.dispatcher_.deferredDelete(std::move(clientToDestroy));
handleSuccess(false);
}
}
Expand Down Expand Up @@ -418,8 +424,10 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onInterval() {
}

void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onTimeout() {
Network::ClientConnectionPtr clientToDestroy(std::move(client_));
host_->setActiveHealthFailureType(Host::ActiveHealthFailureType::TIMEOUT);
client_->close(Network::ConnectionCloseType::NoFlush);
clientToDestroy->close(Network::ConnectionCloseType::NoFlush);
parent_.dispatcher_.deferredDelete(std::move(clientToDestroy));
}

GrpcHealthCheckerImpl::GrpcHealthCheckerImpl(const Cluster& cluster,
Expand Down
27 changes: 27 additions & 0 deletions test/common/upstream/health_checker_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2537,6 +2537,33 @@ TEST_F(TcpHealthCheckerImplTest, PassiveFailureCrossThreadRemoveClusterRace) {
EXPECT_EQ(0UL, cluster_->info_->stats_store_.counter("health_check.passive_failure").value());
}

TEST_F(TcpHealthCheckerImplTest, ConnectionLocalFailure) {
InSequence s;

setupData();
cluster_->prioritySet().getMockHostSet(0)->hosts_ = {
makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")};
expectSessionCreate();
expectClientCreate();
EXPECT_CALL(*connection_, write(_, _));
EXPECT_CALL(*timeout_timer_, enableTimer(_));
health_checker_->start();

// Expect the LocalClose to be handled as a health check failure
EXPECT_CALL(*event_logger_, logUnhealthy(_, _, _, true));
EXPECT_CALL(*timeout_timer_, disableTimer());
EXPECT_CALL(*interval_timer_, enableTimer(_));

// Raise a LocalClose that is not triggered by the health monitor itself.
// e.g. a failure to setsockopt().
connection_->raiseEvent(Network::ConnectionEvent::LocalClose);

EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.attempt").value());
EXPECT_EQ(0UL, cluster_->info_->stats_store_.counter("health_check.success").value());
EXPECT_EQ(1UL, cluster_->info_->stats_store_.counter("health_check.failure").value());
EXPECT_EQ(0UL, cluster_->info_->stats_store_.counter("health_check.passive_failure").value());
}

class TestGrpcHealthCheckerImpl : public GrpcHealthCheckerImpl {
public:
using GrpcHealthCheckerImpl::GrpcHealthCheckerImpl;
Expand Down