Fix AllocationSequence to handle the case when TurnPort stops using shared socket.

webrtc/p2p/client/basicportallocator.cc

 /*
  *  Copyright 2004 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.
  */
 
@@ skipping 1054 matching lines @@
     session_->AddAllocatedPort(port, this, true);
   }
 }
 
 void AllocationSequence::OnReadPacket(
     rtc::AsyncPacketSocket* socket, const char* data, size_t size,
     const rtc::SocketAddress& remote_addr,
     const rtc::PacketTime& packet_time) {
   ASSERT(socket == udp_socket_.get());
 
-  bool turn_port_found = false;
 
   // Try to find the TurnPort that matches the remote address. Note that the
   // message could be a STUN binding response if the TURN server is also used as
   // a STUN server. We don't want to parse every message here to check if it is
   // a STUN binding response, so we pass the message to TurnPort regardless of
   // the message type. The TurnPort will just ignore the message since it will
   // not find any request by transaction ID.
-  for (std::vector<TurnPort*>::const_iterator it = turn_ports_.begin();
-       it != turn_ports_.end(); ++it) {
-    TurnPort* port = *it;
+  for (TurnPort* port : turn_ports_) {
     if (port->server_address().address == remote_addr) {
-      port->HandleIncomingPacket(socket, data, size, remote_addr, packet_time);
-      turn_port_found = true;
-      break;
+      if (port->HandleIncomingPacket(socket, data, size, remote_addr,
+                                     packet_time)) {
+        return;
+      }
     }
   }
 
   if (udp_port_) {
     const ServerAddresses& stun_servers = udp_port_->server_addresses();
 
     // Pass the packet to the UdpPort if there is no matching TurnPort, or if
     // the TURN server is also a STUN server.
-    if (!turn_port_found ||
-        stun_servers.find(remote_addr) != stun_servers.end()) {
-      udp_port_->HandleIncomingPacket(
-          socket, data, size, remote_addr, packet_time);
+    if (stun_servers.find(remote_addr) != stun_servers.end()) {

[honghaiz3, 2016/04/22 01:49:03]

I think this is incorrect. 
In the current code, a packet coming from the other peer directly will be routed
through the UDP port (because of the condition (!turn_port_found). 
With the new change, that packet will be dropped.

[honghaiz3, 2016/04/22 03:52:33]

One possible approach is to use the existing logic; if the source addr is not
found in turn servers or it is found in stun servers, pass it to the udp port.
But then every turn channel packet is sent to udp port (and will be dropped
there). 
A better approach is, if the source address is not found in turn servers or (it
is found there but not consumed and it is also found in stun servers), pass it
to the udp port. 

[Sergey Ulanov, 2016/04/22 18:05:52]

Implemented this suggestion in Patch Set 4.
I'm not sure it it's actually necessary to check if the address in the list of
STUN servers. I think the logic could be simplified:
 1. Find a turn port with the given remote_addr and try passing the new packet
there.
 2. If the packet is not handled by any of the turn ports then pass it to the
udp_port_.

This would simplify the code and I don't think it would make any difference
otherwise. UDPPort::OnReadPacket() should be able to handle the packet
appropriately. I've implemented this approach in Patch Set 5. WDYT?

[honghaiz3, 2016/04/22 19:17:27]

I think patch set 4 is slightly safer. 
Although patch set 5 looks nicer, it leaves a potential hole: 
For those packets whose source address is found in the turn port addresses but
not consumed by turn port, they will always be passed to the stun port. It has
no problem if it is also found in the stun_server addresses (just like patch set
4). But if it is not found in the stun server addresses, the packets will be
treated as app data in the stun port, which may cause problems later (although
in our implementation, they will be dropped). 

+      RTC_DCHECK(udp_port_->SharedSocket());
+      udp_port_->HandleIncomingPacket(socket, data, size, remote_addr,
+                                      packet_time);
     }
   }
 }
 
 void AllocationSequence::OnPortDestroyed(PortInterface* port) {
   if (udp_port_ == port) {
     udp_port_ = NULL;
     return;
   }
 
@@ skipping 72 matching lines @@
   ServerAddresses servers;
   for (size_t i = 0; i < relays.size(); ++i) {
     if (relays[i].type == turn_type && SupportsProtocol(relays[i], type)) {
       servers.insert(relays[i].ports.front().address);
     }
   }
   return servers;
 }
 
 }  // namespace cricket