inet: fix lwIP UDPEndpoint crash on large packets (project-chip#21897)

isiu-apple · Sep 16, 2022 · c99dc51 · c99dc51
1 parent b6e084a
commit c99dc51
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 52 deletions.
diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp
@@ -276,12 +276,10 @@ void UDPEndPointImplLwIP::Free()
     Release();
 }
 
-void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg)
+void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg, IPPacketInfo * pktInfo)
 {
     if ((mState == State::kListening) && (OnMessageReceived != nullptr))
     {
-        const IPPacketInfo * pktInfo = GetPacketInfo(msg);
-
         if (pktInfo != nullptr)
         {
             const IPPacketInfo pktInfoCopy = *pktInfo; // copy the address info so that the app can free the
@@ -296,6 +294,7 @@ void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg)
             }
         }
     }
+    Platform::Delete(pktInfo);
 }
 
 CHIP_ERROR UDPEndPointImplLwIP::GetPCB(IPAddressType addrType)
@@ -364,38 +363,49 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb
 {
     Platform::UniquePtr<struct pbuf> pbufFreeGuard(p);
     UDPEndPointImplLwIP * ep = static_cast<UDPEndPointImplLwIP *>(arg);
-    IPPacketInfo * pktInfo   = nullptr;
     if (ep->mState == State::kClosed)
     {
         return;
     }
+    // Raw pointer is required for passing into lambda.
+    // The memory life cycle of `pktInfo` is manually managed.
+    IPPacketInfo * pktInfo = Platform::New<IPPacketInfo>();
+    if (pktInfo == nullptr)
+    {
+        ChipLogError(Inet, "Cannot allocate packet info");
+        return;
+    }
+
     // TODO: Skip copying the buffer if the pbuf already meets the PacketBuffer memory model
-    System::PacketBufferHandle buf = System::PacketBufferHandle::New(p->tot_len);
+    System::PacketBufferHandle buf = System::PacketBufferHandle::New(p->tot_len, 0);
     if (buf.IsNull() || pbuf_copy_partial(p, buf->Start(), p->tot_len, 0) != p->tot_len)
     {
         ChipLogError(Inet, "Cannot copy received pbuf of size %u", p->tot_len);
+        return;
     }
     buf->SetDataLength(p->tot_len);
 
-    pktInfo = GetPacketInfo(buf);
-    if (pktInfo != nullptr)
-    {
-        pktInfo->SrcAddress  = IPAddress(*addr);
-        pktInfo->DestAddress = IPAddress(*ip_current_dest_addr());
-        pktInfo->Interface   = InterfaceId(ip_current_netif());
-        pktInfo->SrcPort     = port;
-        pktInfo->DestPort    = pcb->local_port;
-    }
+    pktInfo->SrcAddress  = IPAddress(*addr);
+    pktInfo->DestAddress = IPAddress(*ip_current_dest_addr());
+    pktInfo->Interface   = InterfaceId(ip_current_netif());
+    pktInfo->SrcPort     = port;
+    pktInfo->DestPort    = pcb->local_port;
 
-    CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda([ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf)] {
-        ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p));
+    CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda([ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf), pktInfo] {
+        ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p), pktInfo);
     });
 
     if (err == CHIP_NO_ERROR)
     {
         // If ScheduleLambda() succeeded, it has ownership of the buffer, so we need to release it (without freeing it).
         static_cast<void>(std::move(buf).UnsafeRelease());
     }
+    else
+    {
+        // If ScheduleLambda() succeeded, `pktInfo` will be deleted in `HandleDataReceived`.
+        // Otherwise we delete it here.
+        Platform::Delete(pktInfo);
+    }
 }
 
 CHIP_ERROR UDPEndPointImplLwIP::SetMulticastLoopback(IPVersion aIPVersion, bool aLoopback)
@@ -499,19 +509,5 @@ struct netif * UDPEndPointImplLwIP::FindNetifFromInterfaceId(InterfaceId aInterf
     return (lRetval);
 }
 
-IPPacketInfo * UDPEndPointImplLwIP::GetPacketInfo(const System::PacketBufferHandle & aBuffer)
-{
-    if (!aBuffer->EnsureReservedSize(sizeof(IPPacketInfo) + 3))
-    {
-        return nullptr;
-    }
-
-    uintptr_t lStart           = (uintptr_t) aBuffer->Start();
-    uintptr_t lPacketInfoStart = lStart - sizeof(IPPacketInfo);
-
-    // Align to a 4-byte boundary
-    return reinterpret_cast<IPPacketInfo *>(lPacketInfoStart & ~(sizeof(uint32_t) - 1));
-}
-
 } // namespace Inet
 } // namespace chip
diff --git a/src/inet/UDPEndPointImplLwIP.h b/src/inet/UDPEndPointImplLwIP.h
@@ -55,28 +55,7 @@ class UDPEndPointImplLwIP : public UDPEndPoint, public EndPointStateLwIP
     static struct netif * FindNetifFromInterfaceId(InterfaceId aInterfaceId);
     static CHIP_ERROR LwIPBindInterface(struct udp_pcb * aUDP, InterfaceId intfId);
 
-    void HandleDataReceived(chip::System::PacketBufferHandle && aBuffer);
-
-    /**
-     *  Get LwIP IP layer source and destination addressing information.
-     *
-     *  @param[in]   aBuffer    The packet buffer containing the IP message.
-     *
-     *  @returns  a pointer to the address information on success; otherwise,
-     *            nullptr if there is insufficient space in the packet for
-     *            the address information.
-     *
-     *  When using LwIP information about the packet is 'hidden' in the reserved space before the start of the
-     *  data in the packet buffer. This is necessary because the system layer events only have two arguments,
-     *  which in this case are used to convey the pointer to the end point and the pointer to the buffer.
-     *
-     *  In most cases this trick of storing information before the data works because the first buffer in an
-     *  LwIP IP message contains the space that was used for the Ethernet/IP/UDP headers. However, given the
-     *  current size of the IPPacketInfo structure (40 bytes), it is possible for there to not be enough room
-     *  to store the structure along with the payload in a single packet buffer. In practice, this should only
-     *  happen for extremely large IPv4 packets that arrive without an Ethernet header.
-     */
-    static IPPacketInfo * GetPacketInfo(const chip::System::PacketBufferHandle & aBuffer);
+    void HandleDataReceived(System::PacketBufferHandle && msg, IPPacketInfo * pktInfo);
 
     CHIP_ERROR GetPCB(IPAddressType addrType4);
     static void LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb, struct pbuf * p, const ip_addr_t * addr, u16_t port);