Fix WakeEvent::Open() (#8551)

* Fix WakeEvent::Open()

#### Problem

Some tests were reporting,
```
CHIP:CSL: System wake event notify failed: OS Error 0x02000009: Bad file descriptor

```
but not failing.

#### Change overview

- Fix the problem (setting `mWriteFD` too late in `WakeEvent::Open()`).
- Add error returns and checks so that affected tests actually fail
  without the above fix.

#### Testing

With the added error handling, but without the WakeEvent::Open() change,
numerous tests fail.

* review

* review

* fix missed #else case

src/inet/EndPointBasis.cpp

@@ -39,7 +39,7 @@ void EndPointBasis::InitEndPointBasis(InetLayer & aInetLayer, void * aAppState)
 #endif // CHIP_SYSTEM_CONFIG_USE_LWIP
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
-    mSocket.Init(aInetLayer.SystemLayer()->WatchableEvents());
+    (void) mSocket.Init(aInetLayer.SystemLayer()->WatchableEvents());
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 }
 

src/inet/IPEndPointBasis.cpp

@@ -941,7 +941,7 @@ CHIP_ERROR IPEndPointBasis::GetSocket(IPAddressType aAddressType, int aType, int
         const int fd = ::socket(family, aType, aProtocol);
         if (fd == -1)
             return chip::System::MapErrorPOSIX(errno);
-        mSocket.Attach(fd);
+        ReturnErrorOnFailure(mSocket.Attach(fd));
 
         mAddrType = aAddressType;
 

src/inet/RawEndPoint.cpp

@@ -425,7 +425,7 @@ CHIP_ERROR RawEndPoint::Listen(IPEndPointBasis::OnMessageReceivedFunct onMessage
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
     // Wait for ability to read on this endpoint.
     mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));
-    mSocket.RequestCallbackOnPendingRead();
+    ReturnErrorOnFailure(mSocket.RequestCallbackOnPendingRead());
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
     return CHIP_NO_ERROR;

src/inet/TCPEndPoint.cpp

@@ -300,17 +300,19 @@ CHIP_ERROR TCPEndPoint::Listen(uint16_t backlog)
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
     if (listen(mSocket.GetFD(), backlog) != 0)
+    {
         res = chip::System::MapErrorPOSIX(errno);
+    }
     else
     {
         // Enable non-blocking mode for the socket.
         int flags = fcntl(mSocket.GetFD(), F_GETFL, 0);
         fcntl(mSocket.GetFD(), F_SETFL, flags | O_NONBLOCK);
-    }
 
-    // Wait for ability to read on this endpoint.
-    mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));
-    mSocket.RequestCallbackOnPendingRead();
+        // Wait for ability to read on this endpoint.
+        mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));
+        res = mSocket.RequestCallbackOnPendingRead();
+    }
 
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
@@ -527,15 +529,15 @@ CHIP_ERROR TCPEndPoint::Connect(const IPAddress & addr, uint16_t port, Interface
     {
         State = kState_Connected;
         // Wait for ability to read on this endpoint.
-        mSocket.RequestCallbackOnPendingRead();
+        ReturnErrorOnFailure(mSocket.RequestCallbackOnPendingRead());
         if (OnConnectComplete != nullptr)
             OnConnectComplete(this, CHIP_NO_ERROR);
     }
     else
     {
         State = kState_Connecting;
         // Wait for ability to write on this endpoint.
-        mSocket.RequestCallbackOnPendingWrite();
+        ReturnErrorOnFailure(mSocket.RequestCallbackOnPendingWrite());
     }
 
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
@@ -800,7 +802,7 @@ CHIP_ERROR TCPEndPoint::Send(System::PacketBufferHandle && data, bool push)
         mSendQueue = std::move(data);
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
         // Wait for ability to write on this endpoint.
-        mSocket.RequestCallbackOnPendingWrite();
+        ReturnErrorOnFailure(mSocket.RequestCallbackOnPendingWrite());
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
     }
     else
@@ -1423,7 +1425,11 @@ CHIP_ERROR TCPEndPoint::DriveSending()
             if (mSendQueue.IsNull())
             {
                 // Do not wait for ability to write on this endpoint.
-                mSocket.ClearCallbackOnPendingWrite();
+                err = mSocket.ClearCallbackOnPendingWrite();
+                if (err != CHIP_NO_ERROR)
+                {
+                    break;
+                }
             }
         }
 
@@ -1526,8 +1532,16 @@ void TCPEndPoint::HandleConnectComplete(CHIP_ERROR err)
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
         // Wait for ability to read or write on this endpoint.
-        mSocket.RequestCallbackOnPendingRead();
-        mSocket.RequestCallbackOnPendingWrite();
+        err = mSocket.RequestCallbackOnPendingRead();
+        if (err == CHIP_NO_ERROR)
+        {
+            err = mSocket.RequestCallbackOnPendingWrite();
+        }
+        if (err != CHIP_NO_ERROR)
+        {
+            DoClose(err, false);
+            return;
+        }
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
         if (OnConnectComplete != nullptr)
@@ -1536,7 +1550,9 @@ void TCPEndPoint::HandleConnectComplete(CHIP_ERROR err)
 
     // Otherwise, close the connection with an error.
     else
+    {
         DoClose(err, false);
+    }
 }
 
 CHIP_ERROR TCPEndPoint::DoClose(CHIP_ERROR err, bool suppressCallback)
@@ -1654,8 +1670,7 @@ CHIP_ERROR TCPEndPoint::DoClose(CHIP_ERROR err, bool suppressCallback)
                     ChipLogError(Inet, "SO_LINGER: %d", errno);
             }
 
-            if (mSocket.Close() != 0 && err == CHIP_NO_ERROR)
-                err = chip::System::MapErrorPOSIX(errno);
+            mSocket.Close();
         }
     }
 
@@ -2420,7 +2435,7 @@ CHIP_ERROR TCPEndPoint::GetSocket(IPAddressType addrType)
         const int fd = ::socket(family, SOCK_STREAM | SOCK_FLAGS, 0);
         if (fd == -1)
             return chip::System::MapErrorPOSIX(errno);
-        mSocket.Attach(fd);
+        ReturnErrorOnFailure(mSocket.Attach(fd));
         mAddrType = addrType;
 
         // If creating an IPv6 socket, tell the kernel that it will be IPv6 only.  This makes it
@@ -2447,7 +2462,9 @@ CHIP_ERROR TCPEndPoint::GetSocket(IPAddressType addrType)
 #endif // defined(SO_NOSIGPIPE)
     }
     else if (mAddrType != addrType)
+    {
         return CHIP_ERROR_INCORRECT_STATE;
+    }
 
     return CHIP_NO_ERROR;
 }
@@ -2616,7 +2633,7 @@ void TCPEndPoint::ReceiveData()
             else
                 State = kState_Closing;
             // Do not wait for ability to read on this endpoint.
-            mSocket.ClearCallbackOnPendingRead();
+            (void) mSocket.ClearCallbackOnPendingRead();
             // Call the app's OnPeerClose.
             if (OnPeerClose != nullptr)
                 OnPeerClose(this);
@@ -2713,37 +2730,40 @@ void TCPEndPoint::HandleIncomingConnection()
     if (err == CHIP_NO_ERROR)
     {
         // Put the new end point into the Connected state.
-        conEP->mSocket.Attach(conSocket);
-        conEP->State = kState_Connected;
+        err = conEP->mSocket.Attach(conSocket);
+        if (err == CHIP_NO_ERROR)
+        {
+            conEP->State = kState_Connected;
 #if INET_CONFIG_ENABLE_IPV4
-        conEP->mAddrType = (sa.any.sa_family == AF_INET6) ? kIPAddressType_IPv6 : kIPAddressType_IPv4;
+            conEP->mAddrType = (sa.any.sa_family == AF_INET6) ? kIPAddressType_IPv6 : kIPAddressType_IPv4;
 #else  // !INET_CONFIG_ENABLE_IPV4
-        conEP->mAddrType = kIPAddressType_IPv6;
+            conEP->mAddrType = kIPAddressType_IPv6;
 #endif // !INET_CONFIG_ENABLE_IPV4
-        conEP->Retain();
-
-        // Wait for ability to read on this endpoint.
-        conEP->mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(conEP));
-        conEP->mSocket.RequestCallbackOnPendingRead();
+            conEP->Retain();
 
-        // Call the app's callback function.
-        OnConnectionReceived(this, conEP, peerAddr, peerPort);
+            // Wait for ability to read on this endpoint.
+            conEP->mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(conEP));
+            err = conEP->mSocket.RequestCallbackOnPendingRead();
+            if (err == CHIP_NO_ERROR)
+            {
+                // Call the app's callback function.
+                OnConnectionReceived(this, conEP, peerAddr, peerPort);
+                return;
+            }
+        }
     }
 
     // Otherwise immediately close the connection, clean up and call the app's error callback.
-    else
+    if (conSocket != -1)
+        close(conSocket);
+    if (conEP != nullptr)
     {
-        if (conSocket != -1)
-            close(conSocket);
-        if (conEP != nullptr)
-        {
-            if (conEP->State == kState_Connected)
-                conEP->Release();
+        if (conEP->State == kState_Connected)
             conEP->Release();
-        }
-        if (OnAcceptError != nullptr)
-            OnAcceptError(this, err);
+        conEP->Release();
     }
+    if (OnAcceptError != nullptr)
+        OnAcceptError(this, err);
 }
 
 #if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT

src/inet/UDPEndPoint.cpp

@@ -351,7 +351,7 @@ CHIP_ERROR UDPEndPoint::Listen(OnMessageReceivedFunct onMessageReceived, OnRecei
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
     // Wait for ability to read on this endpoint.
     mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));
-    mSocket.RequestCallbackOnPendingRead();
+    ReturnErrorOnFailure(mSocket.RequestCallbackOnPendingRead());
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
     return CHIP_NO_ERROR;

src/lib/support/CodeUtils.h

@@ -532,6 +532,30 @@ inline void chipDie(void)
 #define VerifyOrDieWithMsg(aCondition, aModule, aMessage, ...)                                                                     \
     nlABORT_ACTION(aCondition, ChipLogDetail(aModule, aMessage, ##__VA_ARGS__))
 
+/**
+ *  @def LogErrorOnFailure(expr)
+ *
+ *  @brief
+ *    Logs a message if the expression returns something different than CHIP_NO_ERROR.
+ *
+ *  Example usage:
+ *
+ *  @code
+ *    ReturnLogErrorOnFailure(channel->SendMsg(msg));
+ *  @endcode
+ *
+ *  @param[in]  expr        A scalar expression to be evaluated against CHIP_NO_ERROR.
+ */
+#define LogErrorOnFailure(expr)                                                                                                    \
+    do                                                                                                                             \
+    {                                                                                                                              \
+        CHIP_ERROR __err = (expr);                                                                                                 \
+        if (__err != CHIP_NO_ERROR)                                                                                                \
+        {                                                                                                                          \
+            ChipLogError(NotSpecified, "%s at %s:%d", ErrorStr(__err), __FILE__, __LINE__);                                        \
+        }                                                                                                                          \
+    } while (false)
+
 #if (__cplusplus >= 201103L)
 
 #ifndef __FINAL

src/platform/Linux/MdnsImpl.cpp

@@ -163,11 +163,17 @@ AvahiWatch * Poller::WatchNew(int fd, AvahiWatchEvent event, AvahiWatchCallback
     VerifyOrDie(callback != nullptr && fd >= 0);
 
     auto watch = std::make_unique<AvahiWatch>();
-    watch->mSocket.Init(*mWatchableEvents)
-        .Attach(fd)
-        .SetCallback(AvahiWatchCallbackTrampoline, reinterpret_cast<intptr_t>(watch.get()))
-        .RequestCallbackOnPendingRead(event & AVAHI_WATCH_IN)
-        .RequestCallbackOnPendingWrite(event & AVAHI_WATCH_OUT);
+    watch->mSocket.Init(*mWatchableEvents);
+    LogErrorOnFailure(watch->mSocket.Attach(fd));
+    watch->mSocket.SetCallback(AvahiWatchCallbackTrampoline, reinterpret_cast<intptr_t>(watch.get()));
+    if (event & AVAHI_WATCH_IN)
+    {
+        LogErrorOnFailure(watch->mSocket.RequestCallbackOnPendingRead());
+    }
+    if (event & AVAHI_WATCH_OUT)
+    {
+        LogErrorOnFailure(watch->mSocket.RequestCallbackOnPendingWrite());
+    }
     watch->mCallback = callback;
     watch->mContext  = context;
     watch->mPoller   = this;
@@ -180,19 +186,19 @@ void Poller::WatchUpdate(AvahiWatch * watch, AvahiWatchEvent event)
 {
     if (event & AVAHI_WATCH_IN)
     {
-        watch->mSocket.RequestCallbackOnPendingRead();
+        LogErrorOnFailure(watch->mSocket.RequestCallbackOnPendingRead());
     }
     else
     {
-        watch->mSocket.ClearCallbackOnPendingRead();
+        LogErrorOnFailure(watch->mSocket.ClearCallbackOnPendingRead());
     }
     if (event & AVAHI_WATCH_OUT)
     {
-        watch->mSocket.RequestCallbackOnPendingWrite();
+        LogErrorOnFailure(watch->mSocket.RequestCallbackOnPendingWrite());
     }
     else
     {
-        watch->mSocket.ClearCallbackOnPendingWrite();
+        LogErrorOnFailure(watch->mSocket.ClearCallbackOnPendingWrite());
     }
 }
 

src/system/SystemLayer.cpp

@@ -92,7 +92,7 @@ CHIP_ERROR Layer::Init()
         return CHIP_ERROR_INCORRECT_STATE;
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
-    mWatchableEvents.Init(*this);
+    ReturnErrorOnFailure(mWatchableEvents.Init(*this));
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
     this->AddEventHandlerDelegate(sSystemEventHandlerDelegate);
@@ -119,7 +119,7 @@ CHIP_ERROR Layer::Shutdown()
     }
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
-    mWatchableEvents.Shutdown();
+    ReturnErrorOnFailure(mWatchableEvents.Shutdown());
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
     this->mLayerState = kLayerState_NotInitialized;

src/system/WakeEvent.cpp

@@ -71,30 +71,21 @@ CHIP_ERROR WakeEvent::Open(WatchableEventManager & watchState)
     if (SetNonBlockingMode(fds[FD_WRITE]) < 0)
         return chip::System::MapErrorPOSIX(errno);
 
+    mWriteFD = fds[FD_WRITE];
+
     mFD.Init(watchState);
-    mFD.Attach(fds[FD_READ]);
+    ReturnErrorOnFailure(mFD.Attach(fds[FD_READ]));
     mFD.SetCallback(Confirm, reinterpret_cast<intptr_t>(this));
-    mFD.RequestCallbackOnPendingRead();
-
-    mWriteFD = fds[FD_WRITE];
+    ReturnErrorOnFailure(mFD.RequestCallbackOnPendingRead());
 
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR WakeEvent::Close()
+void WakeEvent::Close()
 {
-    int res = 0;
-
-    res |= mFD.Close();
-    res |= ::close(mWriteFD);
+    mFD.Close();
+    VerifyOrDie(::close(mWriteFD) == 0);
     mWriteFD = -1;
-
-    if (res < 0)
-    {
-        return chip::System::MapErrorPOSIX(errno);
-    }
-
-    return CHIP_NO_ERROR;
 }
 
 void WakeEvent::Confirm()
@@ -144,16 +135,9 @@ CHIP_ERROR WakeEvent::Open(WatchableEventManager & watchState)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR WakeEvent::Close()
+void WakeEvent::Close()
 {
-    int res = mFD.Close();
-
-    if (res < 0)
-    {
-        return chip::System::MapErrorPOSIX(errno);
-    }
-
-    return CHIP_NO_ERROR;
+    mFD.Close();
 }
 
 void WakeEvent::Confirm()

src/system/WakeEvent.h

@@ -46,7 +46,7 @@ class WakeEvent
 {
 public:
     CHIP_ERROR Open(WatchableEventManager & watchState); /**< Initialize the pipeline */
-    CHIP_ERROR Close();                                  /**< Close both ends of the pipeline. */
+    void Close();                                        /**< Close both ends of the pipeline. */
 
     CHIP_ERROR Notify(); /**< Set the event. */
     void Confirm();      /**< Clear the event. */