Skip to content

Commit

Permalink
Implementing synchronous Connect* methods for BrokeredUdpClientSocket
Browse files Browse the repository at this point in the history 
This change modified Connect(), ConnectUsingNetwork(), and
ConnectUsingDefaultNetwork() in BrokeredUdpClientSocket such that if
a connection does not need to be brokered, these methods create and
connect a socket. If a connection needs to be brokered, the methods
now return net::ERR_NOT_IMPLEMENTED.

There is currently one feature flag configuration where the
network service can be enabled, but QUIC session creation can
still be synchronous. When this configuration is enabled, session
creation calls Connect() on windows instead of ConnectAsync().
Since the original implementation did nothing and returned
net::OK this caused a crash. This change fixes that crash,
and also causes brokered sockets to fail instead of completely
crashing.

(cherry picked from commit 9a4da84)

Bug: 1448725
Change-Id: I4abf892331fbf63e3b9c5e4524af3987c5507d45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575243
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Liza Burakova <liza@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1152675}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4589750
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Liza Burakova <liza@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#421}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Liza Burakova authored and Chromium LUCI CQ committed Jun 6, 2023
1 parent 351e16a commit 247aaa0
Show file tree
Hide file tree
Showing 5 changed files with 193 additions and 16 deletions.
3 changes: 3 additions & 0 deletions services/network/broker_helper_win.cc
View file
Expand Up @@ -37,6 +37,9 @@ void BrokerHelperWin::RefreshNetworkList() {

bool BrokerHelperWin::ShouldBroker(const net::IPAddress& address) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (delegate_) {
return delegate_->ShouldBroker();
}
if (address.IsLoopback())
return true;

Expand Down
25 changes: 23 additions & 2 deletions services/network/broker_helper_win.h
View file
Expand Up @@ -29,10 +29,29 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) BrokerHelperWin

~BrokerHelperWin() override;

// Returns whether a connection to |address| would require the socket creation
// to be brokered.
// Delegate for testing.
class COMPONENT_EXPORT(NETWORK_SERVICE) Delegate {
public:
Delegate() = default;

Delegate(const Delegate&) = delete;
Delegate& operator=(const Delegate&) = delete;

virtual ~Delegate() = default;

virtual bool ShouldBroker() const = 0;
};

// Returns whether a connection to |address| would require the socket
// creation to be brokered.
bool ShouldBroker(const net::IPAddress& address) const;

void SetDelegateForTesting(
std::unique_ptr<BrokerHelperWin::Delegate> delegate) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
delegate_ = std::move(delegate);
}

private:
// NetworkChangeNotifier::NetworkChangeObserver implementation:
void OnNetworkChanged(
Expand All @@ -41,6 +60,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) BrokerHelperWin
void RefreshNetworkList();

net::NetworkInterfaceList interfaces_ GUARDED_BY_CONTEXT(sequence_checker_);
std::unique_ptr<BrokerHelperWin::Delegate> delegate_
GUARDED_BY_CONTEXT(sequence_checker_) = nullptr;

SEQUENCE_CHECKER(sequence_checker_);
};
Expand Down
73 changes: 67 additions & 6 deletions services/network/brokered_udp_client_socket.cc
View file
Expand Up @@ -52,21 +52,44 @@ BrokeredUdpClientSocket::~BrokeredUdpClientSocket() {
}

int BrokeredUdpClientSocket::Connect(const net::IPEndPoint& address) {
NOTREACHED();
return net::OK;
#if BUILDFLAG(IS_WIN)
if (!broker_helper_.ShouldBroker(address.address())) {
return ConnectInternal(address, CONNECT,
net::handles::kInvalidNetworkHandle);
}
#endif
// Brokered sockets can only support asynchronous connections so this does not
// need to be implemented. However, this path can still be hit if the sandbox
// is enabled and a caller attempts to call a synchronous Connect. Callers are
// expected to handle Connect failures themselves so we just return
// ERR_NOT_IMPLEMENTED.
return net::ERR_NOT_IMPLEMENTED;
}

int BrokeredUdpClientSocket::ConnectUsingNetwork(
net::handles::NetworkHandle network,
const net::IPEndPoint& address) {
NOTREACHED();
return net::OK;
#if BUILDFLAG(IS_WIN)
if (!broker_helper_.ShouldBroker(address.address())) {
return ConnectInternal(address, CONNECT_USING_NETWORK, network);
}
#endif
// Similar to Connect(), callers are expected to handles failures themselves
// if they call this method, so return ERR_NOT_IMPLEMENTED.
return net::ERR_NOT_IMPLEMENTED;
}

int BrokeredUdpClientSocket::ConnectUsingDefaultNetwork(
const net::IPEndPoint& address) {
NOTREACHED();
return net::OK;
#if BUILDFLAG(IS_WIN)
if (!broker_helper_.ShouldBroker(address.address())) {
return ConnectInternal(address, CONNECT_USING_DEFAULT_NETWORK,
net::handles::kInvalidNetworkHandle);
}
#endif
// Similar to Connect(), callers are expected to handles failures themselves
// if they call this method, so return ERR_NOT_IMPLEMENTED.
return net::ERR_NOT_IMPLEMENTED;
}

int BrokeredUdpClientSocket::ConnectAsync(
Expand Down Expand Up @@ -125,6 +148,44 @@ int BrokeredUdpClientSocket::ConnectAsyncInternal(
return net::ERR_IO_PENDING;
}

int BrokeredUdpClientSocket::ConnectInternal(
const net::IPEndPoint& address,
WhichConnect which_connect,
net::handles::NetworkHandle network) {
socket_ = std::make_unique<net::UDPClientSocket>(bind_type_, net_log_source_,
network_);

// These options must be set before opening a socket or adopting an opened
// socket.
if (use_non_blocking_io_) {
socket_->UseNonBlockingIO();
}
if (recv_optimization_) {
socket_->EnableRecvOptimization();
}

int set_multicast_rv = socket_->SetMulticastInterface(interface_index_);
if (set_multicast_rv != net::OK) {
return set_multicast_rv;
}
socket_->ApplySocketTag(tag_);
socket_->SetMsgConfirm(set_msg_confirm_);

int connect_rv = net::OK;
switch (which_connect) {
case CONNECT:
connect_rv = socket_->Connect(address);
break;
case CONNECT_USING_NETWORK:
connect_rv = socket_->ConnectUsingNetwork(network, address);
break;
case CONNECT_USING_DEFAULT_NETWORK:
connect_rv = socket_->ConnectUsingDefaultNetwork(address);
break;
}
return connect_rv;
}

int BrokeredUdpClientSocket::DidCompleteCreate(
bool should_broker,
const net::IPEndPoint& address,
Expand Down
12 changes: 12 additions & 0 deletions services/network/brokered_udp_client_socket.h
View file
Expand Up @@ -69,6 +69,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) BrokeredUdpClientSocket
BrokeredUdpClientSocket& operator=(const BrokeredUdpClientSocket&) = delete;

// DatagramClientSocket implementation.
// TODO(https://crbug.com/1444811): Remove Connect, ConnectUsingNetwork, and
// ConnectUsingDefaultNetwork once consumers have been migrated to only call
// Connect*Async methods.
int Connect(const net::IPEndPoint& address) override;
int ConnectUsingNetwork(net::handles::NetworkHandle network,
const net::IPEndPoint& address) override;
Expand Down Expand Up @@ -125,6 +128,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) BrokeredUdpClientSocket
bool get_use_non_blocking_io_for_testing() {
return socket_->get_use_non_blocking_io_for_testing();
}
void SetBrokerHelperDelegateForTesting(
std::unique_ptr<BrokerHelperWin::Delegate> delegate) {
broker_helper_.SetDelegateForTesting(std::move(delegate));
}
#endif

private:
Expand All @@ -135,6 +142,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) BrokeredUdpClientSocket
WhichConnect which_connect,
net::handles::NetworkHandle network,
net::CompletionOnceCallback callback);
// Synchronously creates and connects a socket. This method can only be used
// on Windows if a connection does not need to be brokered.
int ConnectInternal(const net::IPEndPoint& address,
WhichConnect which_connect,
net::handles::NetworkHandle network);
// Returns a net error result upon opening and connecting `socket_`. If a
// connection needs to be brokered, the return value is ignored as callback is
// run with the return value instead.
Expand Down
96 changes: 88 additions & 8 deletions services/network/brokered_udp_client_socket_unittest.cc
View file
Expand Up @@ -32,12 +32,33 @@
#include "net/base/network_change_notifier.h"
#endif

#if BUILDFLAG(IS_WIN)
#include "services/network/broker_helper_win.h"
#endif

using net::test::IsError;
using net::test::IsOk;
using testing::Not;

namespace network {

#if BUILDFLAG(IS_WIN)
// A BrokerHelper delegate to manually set whether a socket needs to be
// brokered. This is necessary to make sure we can test connecting unbrokered
// sockets on Windows, since otherwise ShouldBroker would return true for
// localhost addresses.
class TestBrokerHelperDelegate : public BrokerHelperWin::Delegate {
public:
explicit TestBrokerHelperDelegate(bool should_broker)
: should_broker_(should_broker) {}

bool ShouldBroker() const override { return should_broker_; }

private:
bool should_broker_;
};
#endif

// This class's only purpose is to return a BrokeredUdpClientSocket instead of a
// DatagramClientSocket. This is necessary as BrokeredUdpClientSocket has
// specific helper methods for unit tests that DatagramclientSocket does not
Expand Down Expand Up @@ -155,11 +176,11 @@ class BrokeredUdpClientSocketTest : public testing::Test,

const int BrokeredUdpClientSocketTest::kMaxRead;

TEST_F(BrokeredUdpClientSocketTest, FailedConnect) {
TEST_F(BrokeredUdpClientSocketTest, FailedConnectAsync) {
net::TestCompletionCallback callback;
base::test::ScopedDisableRunLoopTimeout disable_timeout;
net::IPEndPoint server_address(net::IPAddress::IPv4Localhost(),
8080 /* port */);
/*port=*/8080);

socket_broker_impl_.SetConnectionFailure(true);

Expand All @@ -170,11 +191,11 @@ TEST_F(BrokeredUdpClientSocketTest, FailedConnect) {
EXPECT_EQ(rv, net::ERR_CONNECTION_FAILED);
}

TEST_F(BrokeredUdpClientSocketTest, Connect) {
TEST_F(BrokeredUdpClientSocketTest, ConnectAsync) {
ASSERT_EQ(0, net::GetGlobalUDPSocketCountForTesting());
net::TestCompletionCallback callback;
net::IPEndPoint server_address(net::IPAddress::IPv4Localhost(),
8080 /* port */);
/*port=*/8080);

int rv = socket_->ConnectAsync(server_address, callback.callback());

Expand All @@ -187,10 +208,69 @@ TEST_F(BrokeredUdpClientSocketTest, Connect) {
ASSERT_EQ(0, net::GetGlobalUDPSocketCountForTesting());
}

TEST_F(BrokeredUdpClientSocketTest, Connect) {
ASSERT_EQ(0, net::GetGlobalUDPSocketCountForTesting());
net::TestCompletionCallback callback;
net::IPEndPoint server_address(net::IPAddress::IPv4Localhost(),
/*port=*/8080);
int rv = net::OK;

#if BUILDFLAG(IS_WIN)
// Pretending we don't need to broker a localhost address to be able to
// reliably test connecting synchronously.
socket_->SetBrokerHelperDelegateForTesting(
std::make_unique<TestBrokerHelperDelegate>(false));
rv = socket_->Connect(server_address);
ASSERT_EQ(rv, net::OK);
EXPECT_EQ(net::handles::kInvalidNetworkHandle, socket_->GetBoundNetwork());

// ConnectUsingNetwork and ConnectUsingDefaultNetwork should return
// ERR_NOT_IMPLEMENTED even if brokering is not required on windows.
auto socket2 = client_socket_factory_.CreateBrokeredUdpClientSocket(
net::DatagramSocket::DEFAULT_BIND, net::NetLog::Get(),
net::NetLogSource());
socket2->SetBrokerHelperDelegateForTesting(
std::make_unique<TestBrokerHelperDelegate>(false));
rv = socket2->ConnectUsingNetwork(net::handles::kInvalidNetworkHandle,
server_address);
ASSERT_EQ(rv, net::ERR_NOT_IMPLEMENTED);
EXPECT_EQ(net::handles::kInvalidNetworkHandle, socket2->GetBoundNetwork());

auto socket3 = client_socket_factory_.CreateBrokeredUdpClientSocket(
net::DatagramSocket::DEFAULT_BIND, net::NetLog::Get(),
net::NetLogSource());
socket3->SetBrokerHelperDelegateForTesting(
std::make_unique<TestBrokerHelperDelegate>(false));
rv = socket3->ConnectUsingDefaultNetwork(server_address);
ASSERT_EQ(rv, net::ERR_NOT_IMPLEMENTED);
EXPECT_EQ(net::handles::kInvalidNetworkHandle, socket3->GetBoundNetwork());
#else
rv = socket_->Connect(server_address);
ASSERT_EQ(rv, net::ERR_NOT_IMPLEMENTED);
EXPECT_EQ(net::handles::kInvalidNetworkHandle, socket_->GetBoundNetwork());
#endif

// ConnectUsingNetwork and ConnectUsingDefaultNetwork should also return
// ERR_NOT_IMPLEMENTED on all platforms.
auto socket4 = client_socket_factory_.CreateDatagramClientSocket(
net::DatagramSocket::DEFAULT_BIND, net::NetLog::Get(),
net::NetLogSource());
rv = socket4->ConnectUsingNetwork(net::handles::kInvalidNetworkHandle,
server_address);
ASSERT_EQ(rv, net::ERR_NOT_IMPLEMENTED);
EXPECT_EQ(net::handles::kInvalidNetworkHandle, socket4->GetBoundNetwork());
auto socket5 = client_socket_factory_.CreateDatagramClientSocket(
net::DatagramSocket::DEFAULT_BIND, net::NetLog::Get(),
net::NetLogSource());
rv = socket5->ConnectUsingDefaultNetwork(server_address);
ASSERT_EQ(rv, net::ERR_NOT_IMPLEMENTED);
EXPECT_EQ(net::handles::kInvalidNetworkHandle, socket5->GetBoundNetwork());
}

TEST_F(BrokeredUdpClientSocketTest, SetOptions) {
net::TestCompletionCallback callback;
net::IPEndPoint server_address(net::IPAddress::IPv4Localhost(),
8080 /* port */);
/*port=*/8080);
EXPECT_THAT(socket_->SetMulticastInterface(1), IsOk());
socket_->SetMsgConfirm(true);
socket_->EnableRecvOptimization();
Expand Down Expand Up @@ -219,7 +299,7 @@ TEST_F(BrokeredUdpClientSocketTest, SetOptions) {

net::TestCompletionCallback callback2;
net::IPEndPoint server_address2(net::IPAddress::IPv4AllZeros(),
8080 /* port */);
/*port=*/8080);
EXPECT_THAT(new_socket->SetMulticastInterface(1), IsOk());
new_socket->UseNonBlockingIO();
rv = new_socket->ConnectAsync(server_address2, callback2.callback());
Expand Down Expand Up @@ -250,12 +330,12 @@ TEST_F(BrokeredUdpClientSocketTest, SimpleReadWrite) {
SimpleReadAndWrite(&server);
}

TEST_F(BrokeredUdpClientSocketTest, ConnectUsingNetwork) {
TEST_F(BrokeredUdpClientSocketTest, ConnectUsingNetworkAsync) {
// The specific value of this address doesn't really matter, and no
// server needs to be running here. The test only needs to call
// ConnectUsingNetworkAsync() and won't send any datagrams.
net::IPEndPoint server_address(net::IPAddress::IPv4Localhost(),
8080 /* port */);
/*port=*/8080);
const net::handles::NetworkHandle wrong_network_handle = 65536;
net::TestCompletionCallback callback;
#if BUILDFLAG(IS_ANDROID)
Expand Down

0 comments on commit 247aaa0

Please sign in to comment.