From 41647d019f1c5ebefce142e9230778cfcdff71a7 Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Sun, 29 Sep 2024 23:35:45 +0200 Subject: [PATCH 1/5] feat(pubsub): Silence errors on PubSub connections with a timeout to avoid logging floods --- src/pubsub/ua_pubsub.h | 2 ++ src/pubsub/ua_pubsub_connection.c | 11 ++++++++--- src/pubsub/ua_pubsub_eventloop.c | 11 ++++++++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/pubsub/ua_pubsub.h b/src/pubsub/ua_pubsub.h index 51a86e1e18a..cb8e094bbd3 100644 --- a/src/pubsub/ua_pubsub.h +++ b/src/pubsub/ua_pubsub.h @@ -162,6 +162,8 @@ typedef struct UA_PubSubConnection { UA_UInt16 configurationFreezeCounter; + UA_DateTime silenceErrorUntil; /* Avoid generating too many logs */ + UA_Boolean deleteFlag; /* To be deleted - in addition to the PubSubState */ UA_DelayedCallback dc; /* For delayed freeing */ } UA_PubSubConnection; diff --git a/src/pubsub/ua_pubsub_connection.c b/src/pubsub/ua_pubsub_connection.c index b763ca1ddb9..1c333b69ee0 100644 --- a/src/pubsub/ua_pubsub_connection.c +++ b/src/pubsub/ua_pubsub_connection.c @@ -65,9 +65,14 @@ decodeNetworkMessage(UA_Server *server, UA_ByteString *buffer, size_t *pos, loops_exit: if(!processed) { - UA_LOG_INFO_CONNECTION(server->config.logging, connection, - "Dataset reader not found. Check PublisherId, " - "WriterGroupId and DatasetWriterId"); + UA_DateTime nowM = UA_DateTime_nowMonotonic(); + if(connection->silenceErrorUntil < nowM) { + UA_LOG_INFO_CONNECTION(server->config.logging, connection, + "Dataset reader not found. Check PublisherId, " + "WriterGroupId and DatasetWriterId. " + "(This error is now silenced for 10s.)"); + connection->silenceErrorUntil = nowM + (UA_DateTime)(10.0 * UA_DATETIME_SEC); + } /* Possible multicast scenario: there are multiple connections (with one * or more ReaderGroups) within a multicast group every connection * receives all network messages, even if some of them are not meant for diff --git a/src/pubsub/ua_pubsub_eventloop.c b/src/pubsub/ua_pubsub_eventloop.c index a542eec7183..2d1db067f7a 100644 --- a/src/pubsub/ua_pubsub_eventloop.c +++ b/src/pubsub/ua_pubsub_eventloop.c @@ -253,9 +253,14 @@ PubSubChannelCallback(UA_ConnectionManager *cm, uintptr_t connectionId, } if(!processed) { - UA_LOG_WARNING_CONNECTION(server->config.logging, psc, - "Message received that could not be processed. " - "Check PublisherID, WriterGroupID and DatasetWriterID."); + UA_DateTime nowM = UA_DateTime_nowMonotonic(); + if(psc->silenceErrorUntil < nowM) { + UA_LOG_WARNING_CONNECTION(server->config.logging, psc, + "Message received that could not be processed. " + "Check PublisherID, WriterGroupID and DatasetWriterID. " + "(This error is now silenced for 10s.)"); + psc->silenceErrorUntil = nowM + (UA_DateTime)(10.0 * UA_DATETIME_SEC); + } } UA_UNLOCK(&server->serviceMutex); From 420205d914b5c80b165aa7dce680bd856936b027 Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Mon, 30 Sep 2024 00:31:31 +0200 Subject: [PATCH 2/5] fix(server): Fix handling of ternary logic NULL in EventFilters --- src/server/ua_subscription_events_filter.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/server/ua_subscription_events_filter.c b/src/server/ua_subscription_events_filter.c index 945d67b0ef3..38ed7e8b219 100644 --- a/src/server/ua_subscription_events_filter.c +++ b/src/server/ua_subscription_events_filter.c @@ -29,9 +29,9 @@ static UA_Ternary UA_Ternary_and(UA_Ternary first, UA_Ternary second) { if(first == UA_TERNARY_FALSE || second == UA_TERNARY_FALSE) return UA_TERNARY_FALSE; - if(first == UA_TERNARY_TRUE && second == UA_TERNARY_TRUE) - return UA_TERNARY_TRUE; - return UA_TERNARY_NULL; + if(first == UA_TERNARY_NULL || second == UA_TERNARY_NULL) + return UA_TERNARY_NULL; + return UA_TERNARY_TRUE; } static UA_Ternary @@ -39,7 +39,7 @@ UA_Ternary_or(UA_Ternary first, UA_Ternary second) { if(first == UA_TERNARY_TRUE || second == UA_TERNARY_TRUE) return UA_TERNARY_TRUE; if(first == UA_TERNARY_NULL || second == UA_TERNARY_NULL) - return UA_TERNARY_TRUE; + return UA_TERNARY_NULL; return UA_TERNARY_FALSE; } From c47f6cf02a73a3b84511674a31daa0024dcb771c Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Mon, 30 Sep 2024 15:39:47 +0200 Subject: [PATCH 3/5] fix(client): Fix a length check in ua_client_connect.c --- src/client/ua_client_connect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/ua_client_connect.c b/src/client/ua_client_connect.c index 7b1b2f7db4f..dd76b57d0ae 100644 --- a/src/client/ua_client_connect.c +++ b/src/client/ua_client_connect.c @@ -206,7 +206,7 @@ signActivateSessionRequest(UA_Client *client, UA_SecureChannel *channel, UA_ByteString signData = UA_BYTESTRING_NULL; size_t signDataSize = channel->remoteCertificate.length + client->serverSessionNonce.length; - if(dataToSignSize > MAX_DATA_SIZE) { + if(signDataSize > MAX_DATA_SIZE) { retval = UA_STATUSCODE_BADINTERNALERROR; goto cleanup; } From 72799b04143b1b480a64999ff13db15e82e4fc05 Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Mon, 30 Sep 2024 15:40:33 +0200 Subject: [PATCH 4/5] refactor(pubsub): Unify identical switch cases --- src/pubsub/ua_pubsub_dataset.c | 2 -- src/pubsub/ua_pubsub_writer.c | 14 ++++---------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/pubsub/ua_pubsub_dataset.c b/src/pubsub/ua_pubsub_dataset.c index 34b0592144f..7248146beda 100644 --- a/src/pubsub/ua_pubsub_dataset.c +++ b/src/pubsub/ua_pubsub_dataset.c @@ -669,8 +669,6 @@ UA_PublishedDataSet_create(UA_Server *server, result.configurationVersion.minorVersion = UA_PubSubConfigurationVersionTimeDifference(); switch(newConfig->publishedDataSetType) { case UA_PUBSUB_DATASET_PUBLISHEDEVENTS_TEMPLATE: - res = UA_STATUSCODE_BADNOTSUPPORTED; - break; case UA_PUBSUB_DATASET_PUBLISHEDEVENTS: res = UA_STATUSCODE_BADNOTSUPPORTED; break; diff --git a/src/pubsub/ua_pubsub_writer.c b/src/pubsub/ua_pubsub_writer.c index 4e915beb83a..5a735fb4ebb 100644 --- a/src/pubsub/ua_pubsub_writer.c +++ b/src/pubsub/ua_pubsub_writer.c @@ -854,21 +854,15 @@ UA_DataSetWriter_generateDataSetMessage(UA_Server *server, if((u64)jsonDsm->dataSetMessageContentMask & (u64)UA_JSONDATASETMESSAGECONTENTMASK_METADATAVERSION) { dataSetMessage->header.configVersionMajorVersionEnabled = true; - if(heartbeat){ - dataSetMessage->header.configVersionMajorVersion = 0; - } else { - dataSetMessage->header.configVersionMajorVersion = - currentDataSet->dataSetMetaData.configurationVersion.majorVersion; - } - } - if((u64)jsonDsm->dataSetMessageContentMask & - (u64)UA_JSONDATASETMESSAGECONTENTMASK_METADATAVERSION) { dataSetMessage->header.configVersionMinorVersionEnabled = true; if(heartbeat){ + dataSetMessage->header.configVersionMajorVersion = 0; dataSetMessage->header.configVersionMinorVersion = 0; } else { + dataSetMessage->header.configVersionMajorVersion = + currentDataSet->dataSetMetaData.configurationVersion.majorVersion; dataSetMessage->header.configVersionMinorVersion = - currentDataSet->dataSetMetaData.configurationVersion.minorVersion; + currentDataSet->dataSetMetaData.configurationVersion.minorVersion; } } From c166551073e048d60c262d7465b943ab6abf50cb Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Mon, 30 Sep 2024 15:41:17 +0200 Subject: [PATCH 5/5] refactor(server): Small simplifications suggested by static code analysis --- src/server/ua_server_utils.c | 3 --- src/server/ua_services_nodemanagement.c | 12 ++++++------ src/server/ua_subscription_monitoreditem.c | 5 ++--- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/server/ua_server_utils.c b/src/server/ua_server_utils.c index f70f21efce1..ac0acd5b45a 100644 --- a/src/server/ua_server_utils.c +++ b/src/server/ua_server_utils.c @@ -36,9 +36,6 @@ getNodeType(UA_Server *server, const UA_NodeHead *head) { UA_Boolean inverse; switch(head->nodeClass) { case UA_NODECLASS_OBJECT: - parentRefIndex = UA_REFERENCETYPEINDEX_HASTYPEDEFINITION; - inverse = false; - break; case UA_NODECLASS_VARIABLE: parentRefIndex = UA_REFERENCETYPEINDEX_HASTYPEDEFINITION; inverse = false; diff --git a/src/server/ua_services_nodemanagement.c b/src/server/ua_services_nodemanagement.c index 5a4ca440648..40908b79501 100644 --- a/src/server/ua_services_nodemanagement.c +++ b/src/server/ua_services_nodemanagement.c @@ -375,10 +375,11 @@ typeCheckVariableNode(UA_Server *server, UA_Session *session, } /* Type-check the value */ - if(retval == UA_STATUSCODE_GOOD && - !compatibleValue(server, session, &node->dataType, node->valueRank, - node->arrayDimensionsSize, node->arrayDimensions, - &value.value, NULL, &reason)) { + UA_Boolean compatible = + compatibleValue(server, session, &node->dataType, + node->valueRank, node->arrayDimensionsSize, + node->arrayDimensions, &value.value, NULL, &reason); + if(!compatible) { UA_LOG_NODEID_INFO(&node->head.nodeId, UA_LOG_INFO_SESSION(server->config.logging, session, "AddNode (%.*s): The VariableNode value has " @@ -1587,10 +1588,9 @@ addNode_finish(UA_Server *server, UA_Session *session, const UA_NodeId *nodeId) } cleanup: + UA_NODESTORE_RELEASE(server, node); if(type) UA_NODESTORE_RELEASE(server, type); - if(node) - UA_NODESTORE_RELEASE(server, node); if(retval != UA_STATUSCODE_GOOD) deleteNode(server, *nodeId, true); return retval; diff --git a/src/server/ua_subscription_monitoreditem.c b/src/server/ua_subscription_monitoreditem.c index 95ca6b3dfab..1055e5cc138 100644 --- a/src/server/ua_subscription_monitoreditem.c +++ b/src/server/ua_subscription_monitoreditem.c @@ -276,9 +276,9 @@ UA_Notification_enqueueAndTrigger(UA_Server *server, UA_Notification *n) { mon->triggeredUntil > UA_DateTime_nowMonotonic())) { UA_Notification_enqueueSub(n); mon->triggeredUntil = UA_INT64_MIN; - UA_LOG_DEBUG_SUBSCRIPTION(server->config.logging, mon->subscription, + UA_LOG_DEBUG_SUBSCRIPTION(server->config.logging, sub, "Notification enqueued (Queue size %lu)", - (long unsigned)mon->subscription->notificationQueueSize); + (long unsigned)sub->notificationQueueSize); } /* Insert into the MonitoredItem. This checks the queue size and @@ -430,7 +430,6 @@ UA_Server_registerMonitoredItem(UA_Server *server, UA_MonitoredItem *mon) { UA_Subscription *sub = mon->subscription; if(sub) { mon->monitoredItemId = ++sub->lastMonitoredItemId; - mon->subscription = sub; sub->monitoredItemsSize++; LIST_INSERT_HEAD(&sub->monitoredItems, mon, listEntry); } else {