Change the size of the ICE ufrag to 4 bytes

webrtc/p2p/base/relayserver.cc

Index: webrtc/p2p/base/relayserver.cc
diff --git a/webrtc/p2p/base/relayserver.cc b/webrtc/p2p/base/relayserver.cc
index e098cbcd843b5f5e0d99c2513a9e3dd7c5d704d8..37b3371bf753f25446891bb6c66ffaa30aa46cbf 100644
--- a/webrtc/p2p/base/relayserver.cc
+++ b/webrtc/p2p/base/relayserver.cc
@@ -26,9 +26,6 @@ namespace cricket {
 // By default, we require a ping every 90 seconds.
 const int MAX_LIFETIME = 15 * 60 * 1000;
 
-// The number of bytes in each of the usernames we use.
-const uint32_t USERNAME_LENGTH = 16;
-
 // Calls SendTo on the given socket and logs any bad results.
 void Send(rtc::AsyncPacketSocket* socket, const char* bytes, size_t size,
           const rtc::SocketAddress& addr) {
@@ -263,11 +260,14 @@ void RelayServer::OnExternalPacket(
     return;
   }
 
-  uint32_t length =
-      std::min(static_cast<uint32_t>(username_attr->length()), USERNAME_LENGTH);
+  uint32_t length = static_cast<uint32_t>(username_attr->length());
   std::string username(username_attr->bytes(), length);
+  // The username is the substring before colon in username_attr
+  std::string::size_type colonpos = username.find(':');
+  if (colonpos != std::string::npos) {
+    username = username.substr(0, colonpos);

[pthatcher1, 2016/05/05 18:07:19]

I don't think this is correct.

The place where we put a binding into bindings_ gets a username from code kind
of like this:

  std::string username;
  const StunByteStringAttribute* username_attr =
msg->GetByteString(STUN_ATTR_USERNAME);
  username.append(username_attr->bytes(), username_attr->length());

So we should just make this code match and use the full username, not the
username up to a colon. 

The only thing the code did before that we might remove is set a maximum
username length to 16.  But this is legacy code and it's probably not worth
changing it for that.


In other words, just revert this file.  It doesn't need to be changed.

[Zhi Huang, 2016/05/05 18:27:09]

The reason I made these changes is some unit tests will fail.

I looked into this and I found that username_attr contains two parts (two names
I think) which is separated by a colon and the length of user_name is 2 *
length(ufrag) + 1. When the ufrag length is 16, the length of username_attr is
33. After min(33, 16), the length will be 16, which works fine. However, when
the ufrag length is 4, the length(username_attr) = 9. After min(9, 16), the
length will be 9, which will be a problem.

[pthatcher1, 2016/05/05 20:24:31]

I don't see why a length of 9 would cause this code to fail, but perhaps there
is something I missing.  In that case, we should fix the unittest to use 16
rather than 4.

Which unit test is it?

[Zhi Huang, 2016/05/05 23:10:59]

I changed code like this:
 const uint32_t USERNAME_LENGTH = 16;
 ...
 uint32_t length = 
  std::min(static_cast<uint32_t>(username_attr->length()),       
  USERNAME_LENGTH);
  std::string username(username_attr->bytes(), length);
  // TODO: Check the HMAC.
  // The binding should already be present.
  BindingMap::iterator biter = bindings_.find(username);
  if (biter == bindings_.end()) {
    LOG(LS_WARNING) << "Dropping packet: no binding with username";
    return;
  }

These tests will fail.
 out/Debug/rtc_unittests: PortTest.TestLocalToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestLocalToGturn
 out/Debug/rtc_unittests: PortTest.TestPRNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestLocalToSslTcpGturn
 out/Debug/rtc_unittests: PortTest.TestPRNatToGturn
 out/Debug/rtc_unittests: PortTest.TestSymNatToGturn
 out/Debug/rtc_unittests: PortTest.TestSymNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestARNatToGturn
 out/Debug/rtc_unittests: PortTest.TestARNATNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestConeNatToGturn
 out/Debug/rtc_unittests: PortTest.TestConeNatToTcpGturn

When I change USERNAME_LENGTH = 4, these tests will fail.
out/Debug/rtc_unittests: RelayServerTest.TestRemoteBind
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestBadUsername
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoDestinationAddress
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoData
 out/Debug/rtc_unittests: RelayServerTest.TestSendRaw

Since the USERNAME_LENGTH is only used here, I think its purpose is to extract
the substring from username_attribute. So using the colon to extract the
username might be more flexible.

[pthatcher1, 2016/05/09 23:59:25]

uint32_t length = static_cast<uint32_t>(username_attr->length()));
std::string username(username_attr->bytes(), length);
// TODO: Check the HMAC.
// The binding should already be present.
BindingMap::iterator biter = bindings_.find(username);
if (biter == bindings_.end()) {
  LOG(LS_WARNING) << "Dropping packet: no binding with username";
  return;
}

On 2016/05/05 23:10:59, Zhi Huang wrote:

+  }
   // TODO: Check the HMAC.
-
   // The binding should already be present.
   BindingMap::iterator biter = bindings_.find(username);
   if (biter == bindings_.end()) {