Data race in UDPEndpoint on lwIP and OpenThread

[gjc13]

Problem

One of our vendors reported random crashes in the light app. The app crashes at line UDPEndPointImplLwIP::LwIPReceiveUDPMessage. The ep->Retain(); line reports invalid reference count and aborted.

The current imlementaion of this function will suffer from data race. ep->Retain() will be executed in the lwIP event loop while ep->Release() is executed in the Matter event loop. The following patch has been suggested to the vendor:

--- a/src/inet/UDPEndPointImplLwIP.cpp
+++ b/src/inet/UDPEndPointImplLwIP.cpp
@@ -371,8 +371,8 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb
         pktInfo->DestPort    = pcb->local_port;
     }
 
-    ep->Retain();
     CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda([ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf)] {
+        ep->Retain();
         ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p));
         ep->Release();
     });
@@ -381,10 +381,6 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb
         // 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
-    {
-        ep->Release();
-    }
 }

This is not correct but will resolve the data race issue. After applying the patch the crash no longer happens. Similar logic is found in the OpenThread UDP endpoint implementation so there will be potential bugs as well.

Proposed solution

One of the following solutions can be adpoted:

• Add thread-safe implementation for reference counting.
• Use the lwIP socket API rather than the raw lwIP API so that the received data will be handled in the Matter event loop.
• Simply remove the Retain and Release logic since in practice nobody is releasing the endpoint.

[gjc13]

@kpschoedel How do you think of this issue?

[kpschoedel]

Personally, I would remove Retain/Release now and file a bug to add thread-safe reference counting when there aren't higher priorities. (I would probably add a separate thread-safe version since ReferenceCounted is usually used entirely within the Matter thread.)

• @kghost from 5c73522

[kghost]

I do think the correct solution is expand thread lock into a global matter lock

Use the lwIP socket API rather than the raw lwIP API so that the received data will be handled in the Matter event loop.

This also works, but I wonder if it is able to catch 1.0 timeline

The app crashes at line UDPEndPointImplLwIP::LwIPReceiveUDPMessage. The ep->Retain(); line reports invalid reference count and aborted.

This is the evidence that there is a bug inside thread implementation. It won't crash inside ep->Retain() even if there is racing here.

[gjc13]

This is the evidence that there is a bug inside thread implementation. It won't crash inside ep->Retain() even if there is racing here.

Retain can crash if the incremented counter in a previous Retain is overriden by the decremented value in Release because of data race. It aborts at VerifyOrDie(!kInitRefCount || mRefCount > 0); and we have observed the mRefCount to be zero.