From c96875f310402c508a0ba644dc81eaa3a88eeb91 Mon Sep 17 00:00:00 2001 From: edwinwu Date: Fri, 10 Jun 2022 19:58:10 -0700 Subject: [PATCH] Fixes A go/asan error (heap_use_after_free) found while running //third_party/nearby/connections/implementation:core_internal_test PiperOrigin-RevId: 454288190 --- connections/implementation/base_pcp_handler.h | 2 +- .../implementation/p2p_cluster_pcp_handler.cc | 14 +++--- .../implementation/wifi_hotspot_test.cc | 50 ++++++++----------- 3 files changed, 29 insertions(+), 37 deletions(-) diff --git a/connections/implementation/base_pcp_handler.h b/connections/implementation/base_pcp_handler.h index 305a7c31..a2023edc 100644 --- a/connections/implementation/base_pcp_handler.h +++ b/connections/implementation/base_pcp_handler.h @@ -310,6 +310,7 @@ class BasePcpHandler : public PcpHandler, Mediums* mediums_; EndpointManager* endpoint_manager_; EndpointChannelManager* channel_manager_; + AtomicBoolean stop_{false}; private: struct PendingConnectionInfo { @@ -533,7 +534,6 @@ class BasePcpHandler : public PcpHandler, // advertising. ConnectionListener advertising_listener_; - AtomicBoolean stop_{false}; Pcp pcp_; Strategy strategy_{PcpToStrategy(pcp_)}; Prng prng_; diff --git a/connections/implementation/p2p_cluster_pcp_handler.cc b/connections/implementation/p2p_cluster_pcp_handler.cc index ddb6d827..fe9f6489 100644 --- a/connections/implementation/p2p_cluster_pcp_handler.cc +++ b/connections/implementation/p2p_cluster_pcp_handler.cc @@ -447,7 +447,7 @@ void P2pClusterPcpHandler::BlePeripheralDiscoveredHandler( [this, client, &peripheral, service_id, advertisement_bytes, fast_advertisement]() RUN_ON_PCP_HANDLER_THREAD() { // Make sure we are still discovering before proceeding. - if (!client->IsDiscovering()) { + if (!client->IsDiscovering() || stop_.Get()) { NEARBY_LOGS(WARNING) << "Skipping discovery of BleAdvertisement header " << absl::BytesToHexString(advertisement_bytes.data()) @@ -531,10 +531,9 @@ void P2pClusterPcpHandler::BlePeripheralLostHandler( "p2p-ble-device-lost", [this, client, service_id, &peripheral]() RUN_ON_PCP_HANDLER_THREAD() { // Make sure we are still discovering before proceeding. - if (!client->IsDiscovering()) { - NEARBY_LOGS(WARNING) - << "Ignoring lost BlePeripheral " << peripheral.GetName() - << " because we are no longer discovering."; + if (!client->IsDiscovering() || stop_.Get()) { + NEARBY_LOGS(WARNING) << "Ignoring lost BlePeripheral because we are " + "no longer discovering."; return; } @@ -616,7 +615,7 @@ void P2pClusterPcpHandler::BleV2PeripheralDiscoveredHandler( [this, client, peripheral = std::move(peripheral), service_id, advertisement_bytes, fast_advertisement]() RUN_ON_PCP_HANDLER_THREAD() { // Make sure we are still discovering before proceeding. - if (!client->IsDiscovering()) { + if (!client->IsDiscovering() || stop_.Get()) { NEARBY_LOGS(WARNING) << "Skipping discovery of BleAdvertisement header " << absl::BytesToHexString(advertisement_bytes.data()) @@ -693,6 +692,7 @@ void P2pClusterPcpHandler::BleV2PeripheralDiscoveredHandler( }); } +// TODO(b/222392304): More test coverage. void P2pClusterPcpHandler::BleV2PeripheralLostHandler( ClientProxy* client, BleV2Peripheral peripheral, const std::string& service_id, const ByteArray& advertisement_bytes, @@ -702,7 +702,7 @@ void P2pClusterPcpHandler::BleV2PeripheralLostHandler( [this, client, service_id, peripheral = std::move(peripheral), advertisement_bytes, fast_advertisement]() RUN_ON_PCP_HANDLER_THREAD() { // Make sure we are still discovering before proceeding. - if (!client->IsDiscovering()) { + if (!client->IsDiscovering() || stop_.Get()) { NEARBY_LOGS(WARNING) << "Ignoring lost BlePeripheral " << absl::BytesToHexString(peripheral.GetId().data()) diff --git a/connections/implementation/wifi_hotspot_test.cc b/connections/implementation/wifi_hotspot_test.cc index 14e1e871..1ca3ba74 100644 --- a/connections/implementation/wifi_hotspot_test.cc +++ b/connections/implementation/wifi_hotspot_test.cc @@ -15,36 +15,31 @@ #include #include "gtest/gtest.h" -#include "absl/time/clock.h" -#include "internal/platform/medium_environment.h" #include "connections/implementation/bwu_handler.h" #include "connections/implementation/wifi_hotspot_bwu_handler.h" +#include "internal/platform/medium_environment.h" namespace location { namespace nearby { namespace connections { namespace { -constexpr absl::Duration kWaitDuration = absl::Milliseconds(300); +constexpr absl::Duration kWaitDuration = absl::Milliseconds(1000); } // namespace -class WifiHotspotTest : public testing::Test { +class WifiHotspotTest : public testing::Test { protected: WifiHotspotTest() { env_.Stop(); env_.Start(); } - ~WifiHotspotTest() override { - env_.Stop(); - } + ~WifiHotspotTest() override { env_.Stop(); } MediumEnvironment& env_{MediumEnvironment::Instance()}; }; TEST_F(WifiHotspotTest, CanCreateBwuHandler) { - BwuHandler::BwuNotifications notifications{ - .incoming_connection_cb = {} - }; + BwuHandler::BwuNotifications notifications{.incoming_connection_cb = {}}; ClientProxy client; Mediums mediums; @@ -52,7 +47,7 @@ TEST_F(WifiHotspotTest, CanCreateBwuHandler) { std::make_unique(mediums, notifications); handler->InitializeUpgradedMediumForEndpoint(&client, /*service_id=*/"B", - /*endpoint_id=*/"2"); + /*endpoint_id=*/"2"); handler->RevertInitiatorState(); SUCCEED(); handler.reset(); @@ -65,17 +60,16 @@ TEST_F(WifiHotspotTest, SoftAPBWUInit_STACreateEndpointChannel) { BwuHandler::BwuNotifications notifications_1{ .incoming_connection_cb = - [&accept_latch, &end_latch](ClientProxy* client, - std::unique_ptr - mutable_connection) { + [&accept_latch, &end_latch]( + ClientProxy* client, + std::unique_ptr + mutable_connection) { NEARBY_LOGS(WARNING) << "Server socket connection accept call back"; accept_latch.CountDown(); EXPECT_TRUE(end_latch.Await(kWaitDuration).result()); }, }; - BwuHandler::BwuNotifications notifications_2{ - .incoming_connection_cb = {} - }; + BwuHandler::BwuNotifications notifications_2{.incoming_connection_cb = {}}; ClientProxy client_1, client_2; Mediums mediums_1, mediums_2; ExceptionOr upgrade_frame; @@ -85,19 +79,17 @@ TEST_F(WifiHotspotTest, SoftAPBWUInit_STACreateEndpointChannel) { // client_1 works as Hotspot SoftAP SingleThreadExecutor server_executor; - server_executor.Execute([&handler_1, &client_1, &upgrade_frame, - &start_latch]() { - ByteArray upgrade_path_available_frame = - handler_1->InitializeUpgradedMediumForEndpoint( - &client_1, - /*service_id=*/"A", - /*endpoint_id=*/"1"); - EXPECT_FALSE(upgrade_path_available_frame.Empty()); - - upgrade_frame = parser::FromBytes(upgrade_path_available_frame); - start_latch.CountDown(); - }); + server_executor.Execute( + [&handler_1, &client_1, &upgrade_frame, &start_latch]() { + ByteArray upgrade_path_available_frame = + handler_1->InitializeUpgradedMediumForEndpoint(&client_1, + /*service_id=*/"A", + /*endpoint_id=*/"1"); + EXPECT_FALSE(upgrade_path_available_frame.Empty()); + upgrade_frame = parser::FromBytes(upgrade_path_available_frame); + start_latch.CountDown(); + }); // client_2 works as Hotspot STA which will connect to client_1 SingleThreadExecutor client_executor;