Skip to content

Commit 0c91011

Browse files
htuchhtuch
authored
network: skip socket options and source address for UDS client connections (#4252)
There are no meaningful semantics for socket options and UDS in Envoy; there is a single socket option for UDS, SO_PASSCRED, which doesn't seem to be interesting. Since socket options and source address are specified at cluster level, and a cluster might have a mix of IP and UDS hosts, we need to silently skip rather than reject the config. As a bonus, cleaned up time sources to be fully mocked in connection_impl_test, since we needed this for the new unit tests. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10015. Risk level: Low Testing: Unit tests and corpus entry added. Signed-off-by: Harvey Tuch <[email protected]>
1 parent da1857d commit 0c91011

File tree

5 files changed

+72
-27
lines changed

5 files changed

+72
-27
lines changed

‎source/common/network/connection_impl.cc‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -542,28 +542,32 @@ ClientConnectionImpl::ClientConnectionImpl(
542542
const Network::ConnectionSocket::OptionsSharedPtr& options)
543543
: ConnectionImpl(dispatcher, std::make_unique<ClientSocketImpl>(remote_address),
544544
std::move(transport_socket), false) {
545-
if (!Network::Socket::applyOptions(options, *socket_,
546-
envoy::api::v2::core::SocketOption::STATE_PREBIND)) {
547-
// Set a special error state to ensure asynchronous close to give the owner of the
548-
// ConnectionImpl a chance to add callbacks and detect the "disconnect".
549-
immediate_error_event_ = ConnectionEvent::LocalClose;
550-
// Trigger a write event to close this connection out-of-band.
551-
file_event_->activate(Event::FileReadyType::Write);
552-
return;
553-
}
554-
555-
if (source_address != nullptr) {
556-
const Api::SysCallIntResult result = source_address->bind(fd());
557-
if (result.rc_ < 0) {
558-
ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(),
559-
strerror(result.errno_));
560-
bind_error_ = true;
545+
// There are no meaningful socket options or source address semantics for
546+
// non-IP sockets, so skip.
547+
if (remote_address->ip() != nullptr) {
548+
if (!Network::Socket::applyOptions(options, *socket_,
549+
envoy::api::v2::core::SocketOption::STATE_PREBIND)) {
561550
// Set a special error state to ensure asynchronous close to give the owner of the
562551
// ConnectionImpl a chance to add callbacks and detect the "disconnect".
563552
immediate_error_event_ = ConnectionEvent::LocalClose;
564-
565553
// Trigger a write event to close this connection out-of-band.
566554
file_event_->activate(Event::FileReadyType::Write);
555+
return;
556+
}
557+
558+
if (source_address != nullptr) {
559+
const Api::SysCallIntResult result = source_address->bind(fd());
560+
if (result.rc_ < 0) {
561+
ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(),
562+
strerror(result.errno_));
563+
bind_error_ = true;
564+
// Set a special error state to ensure asynchronous close to give the owner of the
565+
// ConnectionImpl a chance to add callbacks and detect the "disconnect".
566+
immediate_error_event_ = ConnectionEvent::LocalClose;
567+
568+
// Trigger a write event to close this connection out-of-band.
569+
file_event_->activate(Event::FileReadyType::Write);
570+
}
567571
}
568572
}
569573
}

‎test/common/network/connection_impl_test.cc‎

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest,
7777
TestUtility::ipTestParamsToString);
7878

7979
TEST_P(ConnectionImplDeathTest, BadFd) {
80-
DangerousDeprecatedTestTime test_time;
81-
Event::DispatcherImpl dispatcher(test_time.timeSource());
80+
MockTimeSource time_source;
81+
Event::DispatcherImpl dispatcher(time_source);
8282
EXPECT_DEATH_LOG_TO_STDERR(
8383
ConnectionImpl(dispatcher, std::make_unique<ConnectionSocketImpl>(-1, nullptr, nullptr),
8484
Network::Test::createRawBufferSocket(), false),
@@ -89,7 +89,7 @@ class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
8989
public:
9090
void setUpBasicConnection() {
9191
if (dispatcher_.get() == nullptr) {
92-
dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
92+
dispatcher_.reset(new Event::DispatcherImpl(time_source_));
9393
}
9494
listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false);
9595

@@ -152,7 +152,7 @@ class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
152152

153153
MockBufferFactory* factory = new StrictMock<MockBufferFactory>;
154154
dispatcher_.reset(
155-
new Event::DispatcherImpl(test_time_.timeSource(), Buffer::WatermarkFactoryPtr{factory}));
155+
new Event::DispatcherImpl(time_source_, Buffer::WatermarkFactoryPtr{factory}));
156156
// The first call to create a client session will get a MockBuffer.
157157
// Other calls for server sessions will by default get a normal OwnedImpl.
158158
EXPECT_CALL(*factory, create_(_, _))
@@ -169,7 +169,7 @@ class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
169169
}
170170

171171
protected:
172-
DangerousDeprecatedTestTime test_time_;
172+
MockTimeSource time_source_;
173173
Event::DispatcherPtr dispatcher_;
174174
Stats::IsolatedStoreImpl stats_store_;
175175
Network::TcpListenSocket socket_{Network::Test::getAnyAddress(GetParam()), nullptr, true};
@@ -233,7 +233,7 @@ TEST_P(ConnectionImplTest, CloseDuringConnectCallback) {
233233
}
234234

235235
TEST_P(ConnectionImplTest, ImmediateConnectError) {
236-
dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
236+
dispatcher_.reset(new Event::DispatcherImpl(time_source_));
237237

238238
// Using a broadcast/multicast address as the connection destiantion address causes an
239239
// immediate error return from connect().
@@ -805,7 +805,7 @@ TEST_P(ConnectionImplTest, BindFailureTest) {
805805
source_address_ = Network::Address::InstanceConstSharedPtr{
806806
new Network::Address::Ipv6Instance(address_string, 0)};
807807
}
808-
dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
808+
dispatcher_.reset(new Event::DispatcherImpl(time_source_));
809809
listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false);
810810

811811
client_connection_ = dispatcher_->createClientConnection(
@@ -1199,7 +1199,7 @@ class ReadBufferLimitTest : public ConnectionImplTest {
11991199
public:
12001200
void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size) {
12011201
const uint32_t buffer_size = 256 * 1024;
1202-
dispatcher_.reset(new Event::DispatcherImpl(test_time_.timeSource()));
1202+
dispatcher_.reset(new Event::DispatcherImpl(time_source_));
12031203
listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false);
12041204

12051205
client_connection_ = dispatcher_->createClientConnection(
@@ -1270,8 +1270,8 @@ TEST_P(ReadBufferLimitTest, SomeLimit) {
12701270

12711271
class TcpClientConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
12721272
protected:
1273-
TcpClientConnectionImplTest() : dispatcher_(test_time_.timeSource()) {}
1274-
DangerousDeprecatedTestTime test_time_;
1273+
TcpClientConnectionImplTest() : dispatcher_(time_source_) {}
1274+
MockTimeSource time_source_;
12751275
Event::DispatcherImpl dispatcher_;
12761276
};
12771277
INSTANTIATE_TEST_CASE_P(IpVersions, TcpClientConnectionImplTest,
@@ -1309,5 +1309,33 @@ TEST_P(TcpClientConnectionImplTest, BadConnectConnRefused) {
13091309
dispatcher_.run(Event::Dispatcher::RunType::Block);
13101310
}
13111311

1312+
class PipeClientConnectionImplTest : public testing::Test {
1313+
protected:
1314+
PipeClientConnectionImplTest() : dispatcher_(time_source_) {}
1315+
MockTimeSource time_source_;
1316+
Event::DispatcherImpl dispatcher_;
1317+
const std::string path_{TestEnvironment::unixDomainSocketPath("foo")};
1318+
};
1319+
1320+
// Validate we skip setting socket options on UDS.
1321+
TEST_F(PipeClientConnectionImplTest, SkipSocketOptions) {
1322+
auto option = std::make_shared<MockSocketOption>();
1323+
EXPECT_CALL(*option, setOption(_, _)).Times(0);
1324+
auto options = std::make_shared<Socket::Options>();
1325+
options->emplace_back(option);
1326+
ClientConnectionPtr connection = dispatcher_.createClientConnection(
1327+
Utility::resolveUrl("unix://" + path_), Network::Address::InstanceConstSharedPtr(),
1328+
Network::Test::createRawBufferSocket(), options);
1329+
connection->close(ConnectionCloseType::NoFlush);
1330+
}
1331+
1332+
// Validate we skip setting source address.
1333+
TEST_F(PipeClientConnectionImplTest, SkipSourceAddress) {
1334+
ClientConnectionPtr connection = dispatcher_.createClientConnection(
1335+
Utility::resolveUrl("unix://" + path_), Utility::resolveUrl("tcp://1.2.3.4:5"),
1336+
Network::Test::createRawBufferSocket(), nullptr);
1337+
connection->close(ConnectionCloseType::NoFlush);
1338+
}
1339+
13121340
} // namespace Network
13131341
} // namespace Envoy

‎test/mocks/common.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ MockSystemTimeSource::~MockSystemTimeSource() {}
1010
MockMonotonicTimeSource::MockMonotonicTimeSource() {}
1111
MockMonotonicTimeSource::~MockMonotonicTimeSource() {}
1212

13+
MockTimeSource::MockTimeSource() : TimeSource(system_, monotonic_) {}
14+
MockTimeSource::~MockTimeSource() {}
15+
1316
MockTokenBucket::MockTokenBucket() {}
1417
MockTokenBucket::~MockTokenBucket() {}
1518

‎test/mocks/common.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,15 @@ class MockMonotonicTimeSource : public MonotonicTimeSource {
5151
MOCK_METHOD0(currentTime, MonotonicTime());
5252
};
5353

54+
class MockTimeSource : public TimeSource {
55+
public:
56+
MockTimeSource();
57+
~MockTimeSource();
58+
59+
MockSystemTimeSource system_;
60+
MockMonotonicTimeSource monotonic_;
61+
};
62+
5463
class MockTokenBucket : public TokenBucket {
5564
public:
5665
MockTokenBucket();

‎test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5632902623657984‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
static_resources { clusters { name: " " connect_timeout { nanos: 6 } hosts { pipe { path: " " } } health_checks { timeout { nanos: 6 } interval { nanos: 6 } unhealthy_threshold { } healthy_threshold { } tcp_health_check { } } } } cluster_manager { upstream_bind_config { source_address { address: "0.0.0.0" port_value: 0 } freebind { value: true } } } admin { address { pipe { path: " " } } }

0 commit comments

Comments
 (0)