From 734262c765dbd13b34da1d016c82c7d3d2c4c50c Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Mon, 1 Aug 2016 16:37:14 -0700 Subject: [PATCH] Fixing invalid operator< implementation. It was possible that "A < B" and "B < A" both evaluated to true. This manifested as an assert on Windows, and a memory leak on Linux. Note that the concept of "less than" is meaningless for this object. The operator is only needed so the object can be used as a key in an std::map. BUG=webrtc:6068 R=honghaiz@webrtc.org, kjellander@webrtc.org, skvlad@webrtc.org Review URL: https://codereview.webrtc.org/2187913002 . Cr-Commit-Position: refs/heads/master@{#13598} --- webrtc/BUILD.gn | 1 + webrtc/p2p/base/turnserver.cc | 4 +- webrtc/p2p/base/turnserver_unittest.cc | 68 +++++++++++++++++++ .../p2p/client/basicportallocator_unittest.cc | 10 +-- webrtc/webrtc_tests.gypi | 1 + 5 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 webrtc/p2p/base/turnserver_unittest.cc diff --git a/webrtc/BUILD.gn b/webrtc/BUILD.gn index be0e88e442..cef4c4e7f3 100644 --- a/webrtc/BUILD.gn +++ b/webrtc/BUILD.gn @@ -502,6 +502,7 @@ if (rtc_include_tests) { "p2p/base/transportcontroller_unittest.cc", "p2p/base/transportdescriptionfactory_unittest.cc", "p2p/base/turnport_unittest.cc", + "p2p/base/turnserver_unittest.cc", "p2p/client/basicportallocator_unittest.cc", "p2p/stunprober/stunprober_unittest.cc", ] diff --git a/webrtc/p2p/base/turnserver.cc b/webrtc/p2p/base/turnserver.cc index dac7e38184..7dd9e19f63 100644 --- a/webrtc/p2p/base/turnserver.cc +++ b/webrtc/p2p/base/turnserver.cc @@ -10,6 +10,8 @@ #include "webrtc/p2p/base/turnserver.h" +#include // for std::tie + #include "webrtc/p2p/base/asyncstuntcpsocket.h" #include "webrtc/p2p/base/common.h" #include "webrtc/p2p/base/packetsocketfactory.h" @@ -544,7 +546,7 @@ bool TurnServerConnection::operator==(const TurnServerConnection& c) const { } bool TurnServerConnection::operator<(const TurnServerConnection& c) const { - return src_ < c.src_ || dst_ < c.dst_ || proto_ < c.proto_; + return std::tie(src_, dst_, proto_) < std::tie(c.src_, c.dst_, c.proto_); } std::string TurnServerConnection::ToString() const { diff --git a/webrtc/p2p/base/turnserver_unittest.cc b/webrtc/p2p/base/turnserver_unittest.cc new file mode 100644 index 0000000000..a63670b21d --- /dev/null +++ b/webrtc/p2p/base/turnserver_unittest.cc @@ -0,0 +1,68 @@ +/* + * Copyright 2016 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "webrtc/base/gunit.h" +#include "webrtc/base/physicalsocketserver.h" +#include "webrtc/base/virtualsocketserver.h" +#include "webrtc/p2p/base/basicpacketsocketfactory.h" +#include "webrtc/p2p/base/turnserver.h" + +// NOTE: This is a work in progress. Currently this file only has tests for +// TurnServerConnection, a primitive class used by TurnServer. + +namespace cricket { + +class TurnServerConnectionTest : public testing::Test { + public: + TurnServerConnectionTest() : vss_(&pss_), ss_scope_(&vss_) {} + + void ExpectEqual(const TurnServerConnection& a, + const TurnServerConnection& b) { + EXPECT_TRUE(a == b); + EXPECT_FALSE(a < b); + EXPECT_FALSE(b < a); + } + + void ExpectNotEqual(const TurnServerConnection& a, + const TurnServerConnection& b) { + EXPECT_FALSE(a == b); + // We don't care which is less than the other, as long as only one is less + // than the other. + EXPECT_TRUE((a < b) != (b < a)); + } + + protected: + rtc::PhysicalSocketServer pss_; + rtc::VirtualSocketServer vss_; + rtc::SocketServerScope ss_scope_; + // Since this is constructed after |ss_scope_|, it will pick up |ss_scope_|'s + // socket server. + rtc::BasicPacketSocketFactory socket_factory_; +}; + +TEST_F(TurnServerConnectionTest, ComparisonOperators) { + std::unique_ptr socket1( + socket_factory_.CreateUdpSocket(rtc::SocketAddress("1.1.1.1", 1), 0, 0)); + std::unique_ptr socket2( + socket_factory_.CreateUdpSocket(rtc::SocketAddress("2.2.2.2", 2), 0, 0)); + TurnServerConnection connection1(socket2->GetLocalAddress(), PROTO_UDP, + socket1.get()); + TurnServerConnection connection2(socket2->GetLocalAddress(), PROTO_UDP, + socket1.get()); + TurnServerConnection connection3(socket1->GetLocalAddress(), PROTO_UDP, + socket2.get()); + TurnServerConnection connection4(socket2->GetLocalAddress(), PROTO_TCP, + socket1.get()); + ExpectEqual(connection1, connection2); + ExpectNotEqual(connection1, connection3); + ExpectNotEqual(connection1, connection4); +} + +} // namespace cricket diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index b61d131d07..d430cce033 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -1291,17 +1291,9 @@ TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortPrunesIPv4TurnPorts) { rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0)); } -// Test has an assert error on win_x64_dbg and win_dbg. See: webrtc:6068 -#if defined(WEBRTC_WIN) -#define MAYBE_TestEachInterfaceHasItsOwnTurnPorts \ - DISABLED_TestEachInterfaceHasItsOwnTurnPort -#else -#define MAYBE_TestEachInterfaceHasItsOwnTurnPorts \ - TestEachInterfaceHasItsOwnTurnPorts -#endif // Tests that if prune_turn_ports is set, each network interface // will has its own set of TurnPorts based on their priorities. -TEST_F(BasicPortAllocatorTest, MAYBE_TestEachInterfaceHasItsOwnTurnPorts) { +TEST_F(BasicPortAllocatorTest, TestEachInterfaceHasItsOwnTurnPorts) { turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); turn_server_.AddInternalSocket(kTurnUdpIntIPv6Addr, PROTO_UDP); turn_server_.AddInternalSocket(kTurnTcpIntIPv6Addr, PROTO_TCP); diff --git a/webrtc/webrtc_tests.gypi b/webrtc/webrtc_tests.gypi index f1217eba1c..b5da843d81 100644 --- a/webrtc/webrtc_tests.gypi +++ b/webrtc/webrtc_tests.gypi @@ -117,6 +117,7 @@ 'p2p/base/transportdescriptionfactory_unittest.cc', 'p2p/base/tcpport_unittest.cc', 'p2p/base/turnport_unittest.cc', + 'p2p/base/turnserver_unittest.cc', 'p2p/client/basicportallocator_unittest.cc', 'p2p/stunprober/stunprober_unittest.cc', ],