From 155dfb06b8ebcd2b322f8af80976f253f16c1025 Mon Sep 17 00:00:00 2001
From: Julius Pfrommer <julius.pfrommer@web.de>
Date: Sat, 16 Nov 2024 23:20:16 +0100
Subject: [PATCH] fix(core): Stop processing after a synchronous response

Before it was possible that two responses were received at the same
time. If the client is waiting for the first response, then the second
was processed also before returning the control flow to the client.

This lead to situations where a PublishResponse was processed before
notifying the client about a MonitoredItem handle established just
before.

This fixes #5226.
---
 src/client/ua_client.c         |  4 ++++
 src/client/ua_client_connect.c | 30 ++++++++++++++++++++++++++++++
 src/ua_securechannel.c         |  7 +++++++
 src/ua_securechannel.h         |  1 +
 4 files changed, 42 insertions(+)

diff --git a/src/client/ua_client.c b/src/client/ua_client.c
index 167f77eef2f..74686b362b9 100644
--- a/src/client/ua_client.c
+++ b/src/client/ua_client.c
@@ -500,7 +500,11 @@ processMSGResponse(UA_Client *client, UA_UInt32 requestId,
         UA_clear(response, ac->responseType);
         UA_free(ac);
     } else {
+        /* Return a special status code after processing a synchronous message.
+         * This makes the client return control immediately. */
         ac->syncResponse = NULL; /* Indicate that response was received */
+        if(retval == UA_STATUSCODE_GOOD)
+            retval = UA_STATUSCODE_GOODCOMPLETESASYNCHRONOUSLY;
     }
     return retval;
 }
diff --git a/src/client/ua_client_connect.c b/src/client/ua_client_connect.c
index c4094c60cb3..23a5736c663 100644
--- a/src/client/ua_client_connect.c
+++ b/src/client/ua_client_connect.c
@@ -1512,6 +1512,9 @@ verifyClientApplicationURI(const UA_Client *client) {
 #endif
 }
 
+static void
+delayedNetworkCallback(void *application, void *context);
+
 static void
 __Client_networkCallback(UA_ConnectionManager *cm, uintptr_t connectionId,
                          void *application, void **connectionContext,
@@ -1605,6 +1608,21 @@ __Client_networkCallback(UA_ConnectionManager *cm, uintptr_t connectionId,
                                      messageType, requestId, &payload);
         if(copied)
             UA_ByteString_clear(&payload);
+
+        /* Abort after synchronous processing of a message.
+         * Add a delayed callback to process the remaining buffer ASAP. */
+        if(res == UA_STATUSCODE_GOODCOMPLETESASYNCHRONOUSLY) {
+            if(client->channel.unprocessed.length > client->channel.unprocessedOffset &&
+               client->channel.unprocessedDelayed.callback == NULL) {
+                client->channel.unprocessedDelayed.callback = delayedNetworkCallback;
+                client->channel.unprocessedDelayed.application = client;
+                client->channel.unprocessedDelayed.context = &client->channel;
+                UA_EventLoop *el = client->config.eventLoop;
+                el->addDelayedCallback(el, &client->channel.unprocessedDelayed);
+            }
+            res = UA_STATUSCODE_GOOD;
+            break;
+        }
     }
     res |= UA_SecureChannel_persistBuffer(&client->channel);
 
@@ -1639,6 +1657,18 @@ __Client_networkCallback(UA_ConnectionManager *cm, uintptr_t connectionId,
     UA_UNLOCK(&client->clientMutex);
 }
 
+static void
+delayedNetworkCallback(void *application, void *context) {
+    UA_Client *client = (UA_Client*)application;
+    client->channel.unprocessedDelayed.callback = NULL;
+    if(client->channel.state == UA_SECURECHANNELSTATE_CONNECTED)
+        __Client_networkCallback(client->channel.connectionManager,
+                                 client->channel.connectionId,
+                                 client, &context,
+                                 UA_CONNECTIONSTATE_ESTABLISHED,
+                                 &UA_KEYVALUEMAP_NULL, UA_BYTESTRING_NULL);
+}
+
 /* Initialize a TCP connection. Writes the result to client->connectStatus. */
 static void
 initConnect(UA_Client *client) {
diff --git a/src/ua_securechannel.c b/src/ua_securechannel.c
index 431e56dec39..75698737f27 100644
--- a/src/ua_securechannel.c
+++ b/src/ua_securechannel.c
@@ -184,6 +184,13 @@ UA_SecureChannel_clear(UA_SecureChannel *channel) {
         channel->channelContext = NULL;
     }
 
+    /* Remove remaining delayed callback */
+    if(channel->connectionManager &&
+       channel->connectionManager->eventSource.eventLoop) {
+        UA_EventLoop *el = channel->connectionManager->eventSource.eventLoop;
+        el->removeDelayedCallback(el, &channel->unprocessedDelayed);
+    }
+
     /* The EventLoop connection is no longer valid */
     channel->connectionId = 0;
     channel->connectionManager = NULL;
diff --git a/src/ua_securechannel.h b/src/ua_securechannel.h
index 4faea8cead6..9a83a179a18 100644
--- a/src/ua_securechannel.h
+++ b/src/ua_securechannel.h
@@ -154,6 +154,7 @@ struct UA_SecureChannel {
     UA_ByteString unprocessed;
     size_t unprocessedOffset;
     UA_Boolean unprocessedCopied;
+    UA_DelayedCallback unprocessedDelayed;
 
     UA_CertificateVerification *certificateVerification;
     void *processOPNHeaderApplication;