From cc99bc25d8bc8acc944b31d7ebcb5cb54b8fd2c7 Mon Sep 17 00:00:00 2001 From: nisse Date: Thu, 2 Feb 2017 01:31:30 -0800 Subject: [PATCH] Change StunMessage::AddAttribute return type from bool to void. Proper error handling was missing, using VERIFY to crash in debug builds, while release builds would ignore the error and leak the attribute memory. The check of attribute type consistency was changed to a RTC_DCHECK. Also removes a large number of uses of the deprecated VERIFY macro. BUG=webrtc:6424 Review-Url: https://codereview.webrtc.org/2665343002 Cr-Commit-Position: refs/heads/master@{#16413} --- webrtc/p2p/base/relayport.cc | 12 +++++----- webrtc/p2p/base/stun.cc | 13 +++++------ webrtc/p2p/base/stun.h | 6 ++--- webrtc/p2p/base/stun_unittest.cc | 30 ++++++++++++------------- webrtc/p2p/base/turnport.cc | 38 ++++++++++++++++---------------- webrtc/p2p/base/turnserver.cc | 38 ++++++++++++++++---------------- 6 files changed, 66 insertions(+), 71 deletions(-) diff --git a/webrtc/p2p/base/relayport.cc b/webrtc/p2p/base/relayport.cc index bb642372b5..8f7036cc5f 100644 --- a/webrtc/p2p/base/relayport.cc +++ b/webrtc/p2p/base/relayport.cc @@ -580,32 +580,32 @@ int RelayEntry::SendTo(const void* data, size_t size, StunAttribute::CreateByteString(STUN_ATTR_MAGIC_COOKIE); magic_cookie_attr->CopyBytes(TURN_MAGIC_COOKIE_VALUE, sizeof(TURN_MAGIC_COOKIE_VALUE)); - VERIFY(request.AddAttribute(magic_cookie_attr)); + request.AddAttribute(magic_cookie_attr); StunByteStringAttribute* username_attr = StunAttribute::CreateByteString(STUN_ATTR_USERNAME); username_attr->CopyBytes(port_->username_fragment().c_str(), port_->username_fragment().size()); - VERIFY(request.AddAttribute(username_attr)); + request.AddAttribute(username_attr); StunAddressAttribute* addr_attr = StunAttribute::CreateAddress(STUN_ATTR_DESTINATION_ADDRESS); addr_attr->SetIP(addr.ipaddr()); addr_attr->SetPort(addr.port()); - VERIFY(request.AddAttribute(addr_attr)); + request.AddAttribute(addr_attr); // Attempt to lock if (ext_addr_ == addr) { StunUInt32Attribute* options_attr = StunAttribute::CreateUInt32(STUN_ATTR_OPTIONS); options_attr->SetValue(0x1); - VERIFY(request.AddAttribute(options_attr)); + request.AddAttribute(options_attr); } StunByteStringAttribute* data_attr = StunAttribute::CreateByteString(STUN_ATTR_DATA); data_attr->CopyBytes(data, size); - VERIFY(request.AddAttribute(data_attr)); + request.AddAttribute(data_attr); // TODO: compute the HMAC. @@ -792,7 +792,7 @@ void AllocateRequest::Prepare(StunMessage* request) { username_attr->CopyBytes( entry_->port()->username_fragment().c_str(), entry_->port()->username_fragment().size()); - VERIFY(request->AddAttribute(username_attr)); + request->AddAttribute(username_attr); } void AllocateRequest::OnSent() { diff --git a/webrtc/p2p/base/stun.cc b/webrtc/p2p/base/stun.cc index 3d11c71cd3..b66ac0308a 100644 --- a/webrtc/p2p/base/stun.cc +++ b/webrtc/p2p/base/stun.cc @@ -16,7 +16,6 @@ #include "webrtc/base/byteorder.h" #include "webrtc/base/checks.h" -#include "webrtc/base/common.h" #include "webrtc/base/crc32.h" #include "webrtc/base/logging.h" #include "webrtc/base/messagedigest.h" @@ -74,11 +73,10 @@ bool StunMessage::SetTransactionID(const std::string& str) { return true; } -bool StunMessage::AddAttribute(StunAttribute* attr) { +void StunMessage::AddAttribute(StunAttribute* attr) { // Fail any attributes that aren't valid for this type of message. - if (attr->value_type() != GetAttributeValueType(attr->type())) { - return false; - } + RTC_DCHECK_EQ(attr->value_type(), GetAttributeValueType(attr->type())); + attrs_->push_back(attr); attr->SetOwner(this); size_t attr_length = attr->length(); @@ -86,7 +84,6 @@ bool StunMessage::AddAttribute(StunAttribute* attr) { attr_length += (4 - (attr_length % 4)); } length_ += static_cast(attr_length + 4); - return true; } const StunAddressAttribute* StunMessage::GetAddress(int type) const { @@ -220,7 +217,7 @@ bool StunMessage::AddMessageIntegrity(const char* key, StunByteStringAttribute* msg_integrity_attr = new StunByteStringAttribute(STUN_ATTR_MESSAGE_INTEGRITY, std::string(kStunMessageIntegritySize, '0')); - VERIFY(AddAttribute(msg_integrity_attr)); + AddAttribute(msg_integrity_attr); // Calculate the HMAC for the message. ByteBufferWriter buf; @@ -281,7 +278,7 @@ bool StunMessage::AddFingerprint() { // it can't fail. StunUInt32Attribute* fingerprint_attr = new StunUInt32Attribute(STUN_ATTR_FINGERPRINT, 0); - VERIFY(AddAttribute(fingerprint_attr)); + AddAttribute(fingerprint_attr); // Calculate the CRC-32 for the message and insert it. ByteBufferWriter buf; diff --git a/webrtc/p2p/base/stun.h b/webrtc/p2p/base/stun.h index 153f63dc58..028787c72a 100644 --- a/webrtc/p2p/base/stun.h +++ b/webrtc/p2p/base/stun.h @@ -158,10 +158,8 @@ class StunMessage { const StunErrorCodeAttribute* GetErrorCode() const; const StunUInt16ListAttribute* GetUnknownAttributes() const; - // Takes ownership of the specified attribute, verifies it is of the correct - // type, and adds it to the message. The return value indicates whether this - // was successful. - bool AddAttribute(StunAttribute* attr); + // Takes ownership of the specified attribute and adds it to the message. + void AddAttribute(StunAttribute* attr); // Validates that a raw STUN message has a correct MESSAGE-INTEGRITY value. // This can't currently be done on a StunMessage, since it is affected by diff --git a/webrtc/p2p/base/stun_unittest.cc b/webrtc/p2p/base/stun_unittest.cc index 1624e689d7..f12fa82cde 100644 --- a/webrtc/p2p/base/stun_unittest.cc +++ b/webrtc/p2p/base/stun_unittest.cc @@ -885,7 +885,7 @@ TEST_F(StunTest, WriteMessageWithIPv6AddressAttribute) { StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS); rtc::SocketAddress test_addr(test_ip, kTestMessagePort2); addr->SetAddress(test_addr); - EXPECT_TRUE(msg.AddAttribute(addr)); + msg.AddAttribute(addr); CheckStunHeader(msg, STUN_BINDING_REQUEST, (size - 20)); @@ -915,7 +915,7 @@ TEST_F(StunTest, WriteMessageWithIPv4AddressAttribute) { StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS); rtc::SocketAddress test_addr(test_ip, kTestMessagePort4); addr->SetAddress(test_addr); - EXPECT_TRUE(msg.AddAttribute(addr)); + msg.AddAttribute(addr); CheckStunHeader(msg, STUN_BINDING_RESPONSE, (size - 20)); @@ -945,7 +945,7 @@ TEST_F(StunTest, WriteMessageWithIPv6XorAddressAttribute) { StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS); rtc::SocketAddress test_addr(test_ip, kTestMessagePort1); addr->SetAddress(test_addr); - EXPECT_TRUE(msg.AddAttribute(addr)); + msg.AddAttribute(addr); CheckStunHeader(msg, STUN_BINDING_RESPONSE, (size - 20)); @@ -976,7 +976,7 @@ TEST_F(StunTest, WriteMessageWithIPv4XoreAddressAttribute) { StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS); rtc::SocketAddress test_addr(test_ip, kTestMessagePort3); addr->SetAddress(test_addr); - EXPECT_TRUE(msg.AddAttribute(addr)); + msg.AddAttribute(addr); CheckStunHeader(msg, STUN_BINDING_RESPONSE, (size - 20)); @@ -1076,7 +1076,7 @@ TEST_F(StunTest, WriteMessageWithAnErrorCodeAttribute) { StunErrorCodeAttribute* errorcode = StunAttribute::CreateErrorCode(); errorcode->SetCode(kTestErrorCode); errorcode->SetReason(kTestErrorReason); - EXPECT_TRUE(msg.AddAttribute(errorcode)); + msg.AddAttribute(errorcode); CheckStunHeader(msg, STUN_BINDING_ERROR_RESPONSE, (size - 20)); rtc::ByteBufferWriter out; @@ -1099,7 +1099,7 @@ TEST_F(StunTest, WriteMessageWithAUInt16ListAttribute) { list->AddType(0x1U); list->AddType(0x1000U); list->AddType(0xAB0CU); - EXPECT_TRUE(msg.AddAttribute(list)); + msg.AddAttribute(list); CheckStunHeader(msg, STUN_BINDING_REQUEST, (size - 20)); rtc::ByteBufferWriter out; @@ -1120,7 +1120,7 @@ TEST_F(StunTest, WriteMessageWithOriginAttribute) { kStunTransactionIdLength)); StunByteStringAttribute* origin = new StunByteStringAttribute(STUN_ATTR_ORIGIN, kTestOrigin); - EXPECT_TRUE(msg.AddAttribute(origin)); + msg.AddAttribute(origin); rtc::ByteBufferWriter out; EXPECT_TRUE(msg.Write(&out)); @@ -1393,7 +1393,7 @@ TEST_F(StunTest, ReadRelayMessage) { StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS); addr2->SetPort(13); addr2->SetIP(legacy_ip); - EXPECT_TRUE(msg2.AddAttribute(addr2)); + msg2.AddAttribute(addr2); const StunByteStringAttribute* bytes = msg.GetByteString(STUN_ATTR_USERNAME); ASSERT_TRUE(bytes != NULL); @@ -1403,7 +1403,7 @@ TEST_F(StunTest, ReadRelayMessage) { StunByteStringAttribute* bytes2 = StunAttribute::CreateByteString(STUN_ATTR_USERNAME); bytes2->CopyBytes("abcdefghijkl"); - EXPECT_TRUE(msg2.AddAttribute(bytes2)); + msg2.AddAttribute(bytes2); const StunUInt32Attribute* uval = msg.GetUInt32(STUN_ATTR_LIFETIME); ASSERT_TRUE(uval != NULL); @@ -1411,7 +1411,7 @@ TEST_F(StunTest, ReadRelayMessage) { StunUInt32Attribute* uval2 = StunAttribute::CreateUInt32(STUN_ATTR_LIFETIME); uval2->SetValue(11); - EXPECT_TRUE(msg2.AddAttribute(uval2)); + msg2.AddAttribute(uval2); bytes = msg.GetByteString(STUN_ATTR_MAGIC_COOKIE); ASSERT_TRUE(bytes != NULL); @@ -1424,7 +1424,7 @@ TEST_F(StunTest, ReadRelayMessage) { bytes2 = StunAttribute::CreateByteString(STUN_ATTR_MAGIC_COOKIE); bytes2->CopyBytes(reinterpret_cast(TURN_MAGIC_COOKIE_VALUE), sizeof(TURN_MAGIC_COOKIE_VALUE)); - EXPECT_TRUE(msg2.AddAttribute(bytes2)); + msg2.AddAttribute(bytes2); uval = msg.GetUInt32(STUN_ATTR_BANDWIDTH); ASSERT_TRUE(uval != NULL); @@ -1432,7 +1432,7 @@ TEST_F(StunTest, ReadRelayMessage) { uval2 = StunAttribute::CreateUInt32(STUN_ATTR_BANDWIDTH); uval2->SetValue(6); - EXPECT_TRUE(msg2.AddAttribute(uval2)); + msg2.AddAttribute(uval2); addr = msg.GetAddress(STUN_ATTR_DESTINATION_ADDRESS); ASSERT_TRUE(addr != NULL); @@ -1443,7 +1443,7 @@ TEST_F(StunTest, ReadRelayMessage) { addr2 = StunAttribute::CreateAddress(STUN_ATTR_DESTINATION_ADDRESS); addr2->SetPort(13); addr2->SetIP(legacy_ip); - EXPECT_TRUE(msg2.AddAttribute(addr2)); + msg2.AddAttribute(addr2); addr = msg.GetAddress(STUN_ATTR_SOURCE_ADDRESS2); ASSERT_TRUE(addr != NULL); @@ -1454,7 +1454,7 @@ TEST_F(StunTest, ReadRelayMessage) { addr2 = StunAttribute::CreateAddress(STUN_ATTR_SOURCE_ADDRESS2); addr2->SetPort(13); addr2->SetIP(legacy_ip); - EXPECT_TRUE(msg2.AddAttribute(addr2)); + msg2.AddAttribute(addr2); bytes = msg.GetByteString(STUN_ATTR_DATA); ASSERT_TRUE(bytes != NULL); @@ -1463,7 +1463,7 @@ TEST_F(StunTest, ReadRelayMessage) { bytes2 = StunAttribute::CreateByteString(STUN_ATTR_DATA); bytes2->CopyBytes("abcdefg"); - EXPECT_TRUE(msg2.AddAttribute(bytes2)); + msg2.AddAttribute(bytes2); rtc::ByteBufferWriter out; EXPECT_TRUE(msg.Write(&out)); diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc index e5fe8d97f0..9ecc75ea28 100644 --- a/webrtc/p2p/base/turnport.cc +++ b/webrtc/p2p/base/turnport.cc @@ -928,12 +928,12 @@ void TurnPort::SendRequest(StunRequest* req, int delay) { void TurnPort::AddRequestAuthInfo(StunMessage* msg) { // If we've gotten the necessary data from the server, add it to our request. VERIFY(!hash_.empty()); - VERIFY(msg->AddAttribute(new StunByteStringAttribute( - STUN_ATTR_USERNAME, credentials_.username))); - VERIFY(msg->AddAttribute(new StunByteStringAttribute( - STUN_ATTR_REALM, realm_))); - VERIFY(msg->AddAttribute(new StunByteStringAttribute( - STUN_ATTR_NONCE, nonce_))); + msg->AddAttribute(new StunByteStringAttribute( + STUN_ATTR_USERNAME, credentials_.username)); + msg->AddAttribute(new StunByteStringAttribute( + STUN_ATTR_REALM, realm_)); + msg->AddAttribute(new StunByteStringAttribute( + STUN_ATTR_NONCE, nonce_)); VERIFY(msg->AddMessageIntegrity(hash())); } @@ -1082,7 +1082,7 @@ void TurnAllocateRequest::Prepare(StunMessage* request) { StunUInt32Attribute* transport_attr = StunAttribute::CreateUInt32( STUN_ATTR_REQUESTED_TRANSPORT); transport_attr->SetValue(IPPROTO_UDP << 24); - VERIFY(request->AddAttribute(transport_attr)); + request->AddAttribute(transport_attr); if (!port_->hash().empty()) { port_->AddRequestAuthInfo(request); } @@ -1256,8 +1256,8 @@ void TurnRefreshRequest::Prepare(StunMessage* request) { // No attributes need to be included. request->SetType(TURN_REFRESH_REQUEST); if (lifetime_ > -1) { - VERIFY(request->AddAttribute(new StunUInt32Attribute( - STUN_ATTR_LIFETIME, lifetime_))); + request->AddAttribute(new StunUInt32Attribute( + STUN_ATTR_LIFETIME, lifetime_)); } port_->AddRequestAuthInfo(request); @@ -1326,8 +1326,8 @@ TurnCreatePermissionRequest::TurnCreatePermissionRequest( void TurnCreatePermissionRequest::Prepare(StunMessage* request) { // Create the request as indicated in RFC5766, Section 9.1. request->SetType(TURN_CREATE_PERMISSION_REQUEST); - VERIFY(request->AddAttribute(new StunXorAddressAttribute( - STUN_ATTR_XOR_PEER_ADDRESS, ext_addr_))); + request->AddAttribute(new StunXorAddressAttribute( + STUN_ATTR_XOR_PEER_ADDRESS, ext_addr_)); port_->AddRequestAuthInfo(request); } @@ -1387,10 +1387,10 @@ TurnChannelBindRequest::TurnChannelBindRequest( void TurnChannelBindRequest::Prepare(StunMessage* request) { // Create the request as indicated in RFC5766, Section 11.1. request->SetType(TURN_CHANNEL_BIND_REQUEST); - VERIFY(request->AddAttribute(new StunUInt32Attribute( - STUN_ATTR_CHANNEL_NUMBER, channel_id_ << 16))); - VERIFY(request->AddAttribute(new StunXorAddressAttribute( - STUN_ATTR_XOR_PEER_ADDRESS, ext_addr_))); + request->AddAttribute(new StunUInt32Attribute( + STUN_ATTR_CHANNEL_NUMBER, channel_id_ << 16)); + request->AddAttribute(new StunXorAddressAttribute( + STUN_ATTR_XOR_PEER_ADDRESS, ext_addr_)); port_->AddRequestAuthInfo(request); } @@ -1471,10 +1471,10 @@ int TurnEntry::Send(const void* data, size_t size, bool payload, msg.SetType(TURN_SEND_INDICATION); msg.SetTransactionID( rtc::CreateRandomString(kStunTransactionIdLength)); - VERIFY(msg.AddAttribute(new StunXorAddressAttribute( - STUN_ATTR_XOR_PEER_ADDRESS, ext_addr_))); - VERIFY(msg.AddAttribute(new StunByteStringAttribute( - STUN_ATTR_DATA, data, size))); + msg.AddAttribute(new StunXorAddressAttribute( + STUN_ATTR_XOR_PEER_ADDRESS, ext_addr_)); + msg.AddAttribute(new StunByteStringAttribute( + STUN_ATTR_DATA, data, size)); VERIFY(msg.Write(&buf)); // If we're sending real data, request a channel bind that we can use later. diff --git a/webrtc/p2p/base/turnserver.cc b/webrtc/p2p/base/turnserver.cc index eef12ead2d..26d306d519 100644 --- a/webrtc/p2p/base/turnserver.cc +++ b/webrtc/p2p/base/turnserver.cc @@ -113,8 +113,8 @@ static bool InitErrorResponse(const StunMessage* req, int code, return false; resp->SetType(resp_type); resp->SetTransactionID(req->transaction_id()); - VERIFY(resp->AddAttribute(new cricket::StunErrorCodeAttribute( - STUN_ATTR_ERROR_CODE, code, reason))); + resp->AddAttribute(new cricket::StunErrorCodeAttribute( + STUN_ATTR_ERROR_CODE, code, reason)); return true; } @@ -356,7 +356,7 @@ void TurnServer::HandleBindingRequest(TurnServerConnection* conn, StunAddressAttribute* mapped_addr_attr; mapped_addr_attr = new StunXorAddressAttribute( STUN_ATTR_XOR_MAPPED_ADDRESS, conn->src()); - VERIFY(response.AddAttribute(mapped_addr_attr)); + response.AddAttribute(mapped_addr_attr); SendStun(conn, &response); } @@ -470,10 +470,10 @@ void TurnServer::SendErrorResponseWithRealmAndNonce( timestamp = ts_for_next_nonce_; ts_for_next_nonce_ = 0; } - VERIFY(resp.AddAttribute( - new StunByteStringAttribute(STUN_ATTR_NONCE, GenerateNonce(timestamp)))); - VERIFY(resp.AddAttribute(new StunByteStringAttribute( - STUN_ATTR_REALM, realm_))); + resp.AddAttribute( + new StunByteStringAttribute(STUN_ATTR_NONCE, GenerateNonce(timestamp))); + resp.AddAttribute(new StunByteStringAttribute( + STUN_ATTR_REALM, realm_)); SendStun(conn, &resp); } @@ -483,8 +483,8 @@ void TurnServer::SendErrorResponseWithAlternateServer( TurnMessage resp; InitErrorResponse(msg, STUN_ERROR_TRY_ALTERNATE, STUN_ERROR_REASON_TRY_ALTERNATE_SERVER, &resp); - VERIFY(resp.AddAttribute(new StunAddressAttribute( - STUN_ATTR_ALTERNATE_SERVER, addr))); + resp.AddAttribute(new StunAddressAttribute( + STUN_ATTR_ALTERNATE_SERVER, addr)); SendStun(conn, &resp); } @@ -492,8 +492,8 @@ void TurnServer::SendStun(TurnServerConnection* conn, StunMessage* msg) { rtc::ByteBufferWriter buf; // Add a SOFTWARE attribute if one is set. if (!software_.empty()) { - VERIFY(msg->AddAttribute( - new StunByteStringAttribute(STUN_ATTR_SOFTWARE, software_))); + msg->AddAttribute( + new StunByteStringAttribute(STUN_ATTR_SOFTWARE, software_)); } msg->Write(&buf); Send(conn, buf); @@ -658,9 +658,9 @@ void TurnServerAllocation::HandleAllocateRequest(const TurnMessage* msg) { external_socket_->GetLocalAddress()); StunUInt32Attribute* lifetime_attr = new StunUInt32Attribute(STUN_ATTR_LIFETIME, lifetime_secs); - VERIFY(response.AddAttribute(mapped_addr_attr)); - VERIFY(response.AddAttribute(relayed_addr_attr)); - VERIFY(response.AddAttribute(lifetime_attr)); + response.AddAttribute(mapped_addr_attr); + response.AddAttribute(relayed_addr_attr); + response.AddAttribute(lifetime_attr); SendResponse(&response); } @@ -682,7 +682,7 @@ void TurnServerAllocation::HandleRefreshRequest(const TurnMessage* msg) { StunUInt32Attribute* lifetime_attr = new StunUInt32Attribute(STUN_ATTR_LIFETIME, lifetime_secs); - VERIFY(response.AddAttribute(lifetime_attr)); + response.AddAttribute(lifetime_attr); SendResponse(&response); } @@ -819,10 +819,10 @@ void TurnServerAllocation::OnExternalPacket( msg.SetType(TURN_DATA_INDICATION); msg.SetTransactionID( rtc::CreateRandomString(kStunTransactionIdLength)); - VERIFY(msg.AddAttribute(new StunXorAddressAttribute( - STUN_ATTR_XOR_PEER_ADDRESS, addr))); - VERIFY(msg.AddAttribute(new StunByteStringAttribute( - STUN_ATTR_DATA, data, size))); + msg.AddAttribute(new StunXorAddressAttribute( + STUN_ATTR_XOR_PEER_ADDRESS, addr)); + msg.AddAttribute(new StunByteStringAttribute( + STUN_ATTR_DATA, data, size)); server_->SendStun(&conn_, &msg); } else { LOG_J(LS_WARNING, this) << "Received external packet without permission, "