From 247aaa01aec41d1fd3e23b50d43c70370a06c975 Mon Sep 17 00:00:00 2001 From: Liza Burakova Date: Tue, 6 Jun 2023 19:23:47 +0000 Subject: [PATCH] Implementing synchronous Connect* methods for BrokeredUdpClientSocket 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 9a4da846e1b2f14463cc88ed619bd7a7bd8dc38f) Bug: 1448725 Change-Id: I4abf892331fbf63e3b9c5e4524af3987c5507d45 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575243 Reviewed-by: Kenichi Ishibashi Commit-Queue: Liza Burakova Cr-Original-Commit-Position: refs/heads/main@{#1152675} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4589750 Bot-Commit: Rubber Stamper Auto-Submit: Liza Burakova Cr-Commit-Position: refs/branch-heads/5790@{#421} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- services/network/broker_helper_win.cc | 3 + services/network/broker_helper_win.h | 25 ++++- .../network/brokered_udp_client_socket.cc | 73 ++++++++++++-- services/network/brokered_udp_client_socket.h | 12 +++ .../brokered_udp_client_socket_unittest.cc | 96 +++++++++++++++++-- 5 files changed, 193 insertions(+), 16 deletions(-) diff --git a/services/network/broker_helper_win.cc b/services/network/broker_helper_win.cc index 805b94c1f412d..19474a1c43dbd 100644 --- a/services/network/broker_helper_win.cc +++ b/services/network/broker_helper_win.cc @@ -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; diff --git a/services/network/broker_helper_win.h b/services/network/broker_helper_win.h index ccb422e9365f1..bcc260065e1b6 100644 --- a/services/network/broker_helper_win.h +++ b/services/network/broker_helper_win.h @@ -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 delegate) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + delegate_ = std::move(delegate); + } + private: // NetworkChangeNotifier::NetworkChangeObserver implementation: void OnNetworkChanged( @@ -41,6 +60,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) BrokerHelperWin void RefreshNetworkList(); net::NetworkInterfaceList interfaces_ GUARDED_BY_CONTEXT(sequence_checker_); + std::unique_ptr delegate_ + GUARDED_BY_CONTEXT(sequence_checker_) = nullptr; SEQUENCE_CHECKER(sequence_checker_); }; diff --git a/services/network/brokered_udp_client_socket.cc b/services/network/brokered_udp_client_socket.cc index c9e8f9a899498..425a21b0b89ce 100644 --- a/services/network/brokered_udp_client_socket.cc +++ b/services/network/brokered_udp_client_socket.cc @@ -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( @@ -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(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, diff --git a/services/network/brokered_udp_client_socket.h b/services/network/brokered_udp_client_socket.h index 9fc0769f33ea0..ed0e689b2fdbf 100644 --- a/services/network/brokered_udp_client_socket.h +++ b/services/network/brokered_udp_client_socket.h @@ -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; @@ -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 delegate) { + broker_helper_.SetDelegateForTesting(std::move(delegate)); + } #endif private: @@ -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. diff --git a/services/network/brokered_udp_client_socket_unittest.cc b/services/network/brokered_udp_client_socket_unittest.cc index 847137dff2f95..a23db11123979 100644 --- a/services/network/brokered_udp_client_socket_unittest.cc +++ b/services/network/brokered_udp_client_socket_unittest.cc @@ -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 @@ -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); @@ -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()); @@ -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(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(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(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(); @@ -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()); @@ -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)