From 90d69a05233206e1bce5896405bd34545bcd6b40 Mon Sep 17 00:00:00 2001 From: Justin Clarke Casey Date: Fri, 17 Oct 2008 20:14:31 +0000 Subject: [PATCH] * close two potential race conditions where a new asynchronous UDP recieve could overwrite an existing endpoint that had not yet been used by the previous thread * in practice these race conditions were probably pretty rare --- .../ClientStack/LindenUDP/LLUDPServer.cs | 122 ++++++++---------- 1 file changed, 53 insertions(+), 69 deletions(-) diff --git a/OpenSim/Region/ClientStack/LindenUDP/LLUDPServer.cs b/OpenSim/Region/ClientStack/LindenUDP/LLUDPServer.cs index 26a77e43de..01a4dd6db5 100644 --- a/OpenSim/Region/ClientStack/LindenUDP/LLUDPServer.cs +++ b/OpenSim/Region/ClientStack/LindenUDP/LLUDPServer.cs @@ -58,10 +58,11 @@ namespace OpenSim.Region.ClientStack.LindenUDP protected byte[] ZeroBuffer = new byte[8192]; /// - /// The endpoint of a sender of a particular packet. The port is changed by the various socket receive methods + /// This is an endpoint that is reused where we don't need to protect the information from potentially + /// being stomped on by other threads. /// protected EndPoint reusedEpSender = new IPEndPoint(IPAddress.Any, 0); - protected EndPoint reusedEpProxy; + protected int proxyPortOffset; protected AsyncCallback ReceivedData; @@ -178,11 +179,11 @@ namespace OpenSim.Region.ClientStack.LindenUDP Packet packet = null; int numBytes = 1; bool ok = false; - reusedEpSender = new IPEndPoint(IPAddress.Any, 0); + EndPoint epSender = new IPEndPoint(IPAddress.Any, 0); try { - numBytes = m_socket.EndReceiveFrom(result, ref reusedEpSender); + numBytes = m_socket.EndReceiveFrom(result, ref epSender); ok = true; } catch (SocketException e) @@ -218,12 +219,6 @@ namespace OpenSim.Region.ClientStack.LindenUDP for (z = numBytes ; z < RecvBuffer.Length ; z++) RecvBuffer[z] = 0; - reusedEpProxy = reusedEpSender; - if (proxyPortOffset != 0) - { - reusedEpSender = ProxyCodec.DecodeProxyMessage(RecvBuffer, ref numBytes); - } - int packetEnd = numBytes - 1; try @@ -242,30 +237,48 @@ namespace OpenSim.Region.ClientStack.LindenUDP { m_log.Debug("[CLIENT]: " + e); } + } + + EndPoint epProxy = null; + + // If we've received a use circuit packet, then we need to decode an endpoint proxy, if one exists, before + // allowing the RecvBuffer to be overwritten by the next packet. + if (packet.Type == PacketType.UseCircuitCode) + { + epProxy = epSender; + if (proxyPortOffset != 0) + { + epSender = ProxyCodec.DecodeProxyMessage(RecvBuffer, ref numBytes); + } } - - BeginReceive(); + + BeginReceive(); if (packet != null) - ProcessInPacket(packet); + { + if (packet.Type == PacketType.UseCircuitCode) + AddNewClient((UseCircuitCodePacket)packet, epSender, epProxy); + else + ProcessInPacket(packet, epSender); + } } /// - /// Process a successfully received packet. We pass the packet on to the LLPacketServer - /// except in the case that the packet is UseCircuitCode. In that case we set up the circuit code instead. + /// Process a successfully received packet. /// /// - protected virtual void ProcessInPacket(Packet packet) + /// + protected virtual void ProcessInPacket(Packet packet, EndPoint epSender) { try { // do we already have a circuit for this endpoint uint circuit; - bool ret; + lock (clientCircuits) { - ret = clientCircuits.TryGetValue(reusedEpSender, out circuit); + ret = clientCircuits.TryGetValue(epSender, out circuit); } if (ret) @@ -275,21 +288,6 @@ namespace OpenSim.Region.ClientStack.LindenUDP m_packetServer.InPacket(circuit, packet); } - else if (packet.Type == PacketType.UseCircuitCode) - { - UseCircuitCodePacket p = (UseCircuitCodePacket)packet; - - AddNewClient(p); - - // Ack the first UseCircuitCode packet - PacketAckPacket ack_it = (PacketAckPacket)PacketPool.Instance.GetPacket(PacketType.PacketAck); - // TODO: don't create new blocks if recycling an old packet - ack_it.Packets = new PacketAckPacket.PacketsBlock[1]; - ack_it.Packets[0] = new PacketAckPacket.PacketsBlock(); - ack_it.Packets[0].ID = packet.Header.Sequence; - ack_it.Header.Reliable = false; - SendPacketTo(ack_it.ToBytes(), ack_it.ToBytes().Length, SocketFlags.None, p.CircuitCode.Code); - } } catch (Exception e) { @@ -319,7 +317,7 @@ namespace OpenSim.Region.ClientStack.LindenUDP } catch (Exception a) { - m_log.Info("[UDPSERVER]: " + a); + m_log.Error("[UDPSERVER]: " + a); } try @@ -356,61 +354,47 @@ namespace OpenSim.Region.ClientStack.LindenUDP /// Add a new client circuit. /// /// - protected virtual void AddNewClient(UseCircuitCodePacket useCircuit) + /// + /// + protected virtual void AddNewClient(UseCircuitCodePacket useCircuit, EndPoint epSender, EndPoint epProxy) { //Slave regions don't accept new clients if (m_localScene.Region_Status != RegionStatus.SlaveScene) { - m_log.DebugFormat( - "[CLIENT]: Adding new circuit for agent {0}, circuit code {1}", - useCircuit.CircuitCode.ID, useCircuit.CircuitCode.Code); - - // Copy the current reusedEpSender and reusedEpProxy to store in the maps and hashes, - // since the reused ones will change on the next packet receipt. - IPEndPoint reusedIpEpSender = (IPEndPoint)reusedEpSender; - EndPoint epSender = new IPEndPoint(reusedIpEpSender.Address, reusedIpEpSender.Port); - - IPEndPoint reusedIpEpPorxy = (IPEndPoint)reusedEpProxy; - EndPoint epProxy = new IPEndPoint(reusedIpEpPorxy.Address, reusedIpEpPorxy.Port); - lock (clientCircuits) { - if (!clientCircuits.ContainsKey(epSender)) - { - clientCircuits.Add(epSender, useCircuit.CircuitCode.Code); - } - else + if (!clientCircuits.ContainsKey(epSender)) { - m_log.Error( - "[CLIENT]: clientCircuits already contains entry for user " - + useCircuit.CircuitCode.Code + ". NOT adding."); + m_log.DebugFormat( + "[CLIENT]: Adding new circuit for agent {0}, circuit code {1}", + useCircuit.CircuitCode.ID, useCircuit.CircuitCode.Code); + + clientCircuits.Add(epSender, useCircuit.CircuitCode.Code); } } // This doesn't need locking as it's synchronized data if (!clientCircuits_reverse.ContainsKey(useCircuit.CircuitCode.Code)) clientCircuits_reverse.Add(useCircuit.CircuitCode.Code, epSender); - else - m_log.Error( - "[CLIENT]: clientCurcuits_reverse already contains entry for user " - + useCircuit.CircuitCode.Code + ". NOT adding."); lock (proxyCircuits) { if (!proxyCircuits.ContainsKey(useCircuit.CircuitCode.Code)) proxyCircuits.Add(useCircuit.CircuitCode.Code, epProxy); - else - m_log.Error( - "[CLIENT]: proxyCircuits already contains entry for user " - + useCircuit.CircuitCode.Code + ". NOT adding."); } - if (!PacketServer.AddNewClient(epSender, useCircuit, m_assetCache, m_circuitManager, epProxy)) - m_log.ErrorFormat( - "[CLIENT]: A circuit already existed for agent {0}, circuit {1}", - useCircuit.CircuitCode.ID, useCircuit.CircuitCode.Code); - } - + PacketServer.AddNewClient(epSender, useCircuit, m_assetCache, m_circuitManager, epProxy); + } + + // Ack the UseCircuitCode packet + PacketAckPacket ack_it = (PacketAckPacket)PacketPool.Instance.GetPacket(PacketType.PacketAck); + // TODO: don't create new blocks if recycling an old packet + ack_it.Packets = new PacketAckPacket.PacketsBlock[1]; + ack_it.Packets[0] = new PacketAckPacket.PacketsBlock(); + ack_it.Packets[0].ID = useCircuit.Header.Sequence; + ack_it.Header.Reliable = false; + SendPacketTo(ack_it.ToBytes(), ack_it.ToBytes().Length, SocketFlags.None, useCircuit.CircuitCode.Code); + PacketPool.Instance.ReturnPacket(useCircuit); }