Fix a case where NodeId.Null is modified (#2995)

* Add tests to catch if NodeId.Null is modified in DEBUG, fix an issue where NodeId.Null was overwritten

* add (C)

* only enable immutable NodeId to catch issues

* typos and comments

Author: mregen

Stack/Opc.Ua.Core/Types/BuiltIn/ExtensionObject.cs

@@ -717,7 +717,8 @@ private NodeId XmlEncodedTypeId
                 // check for null Id.
                 if (m_typeId.IsNull)
                 {
-                    return NodeId.Null;
+                    // note: this NodeId is modified when the ExtensionObject is deserialized.
+                    return new NodeId();
                 }
 
                 return ExpandedNodeId.ToNodeId(m_typeId, m_context.NamespaceUris);

Stack/Opc.Ua.Core/Types/BuiltIn/Matrix.cs

@@ -1,6 +1,17 @@
+/* Copyright (c) 1996-2022 The OPC Foundation. All rights reserved.
+   The source code in this file is covered under a dual-license scenario:
+     - RCL: for OPC Foundation Corporate Members in good-standing
+     - GPL V2: everybody else
+   RCL license terms accompanied with this source code. See http://opcfoundation.org/License/RCL/1.00/
+   GNU General Public License as published by the Free Software Foundation;
+   version 2 of the License are accompanied with this source code. See http://opcfoundation.org/License/GPLv2
+   This source code is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+*/
+
 using System;
 using System.Diagnostics;
-using System.Globalization;
 using System.Runtime.Serialization;
 using System.Text;
 
@@ -30,7 +41,6 @@ public Matrix(Array value, BuiltInType builtInType)
 
             m_elements = Utils.FlattenArray(value);
             m_typeInfo = new TypeInfo(builtInType, m_dimensions.Length);
-
         }
 
         /// <summary>
@@ -61,7 +71,6 @@ public Matrix(Array elements, BuiltInType builtInType, params int[] dimensions)
 
             SanityCheckArrayElements(m_elements, builtInType);
         }
-
         #endregion
 
         #region Public Members
@@ -242,16 +251,14 @@ public virtual object Clone()
         /// <param name="builtInType">The builtInType used for the elements.</param>
         [Conditional("DEBUG")]
         private static void SanityCheckArrayElements(Array elements, BuiltInType builtInType)
-
-
         {
 #if DEBUG
             TypeInfo sanityCheck = TypeInfo.Construct(elements);
             Debug.Assert(sanityCheck.BuiltInType == builtInType || builtInType == BuiltInType.Enumeration ||
-                    (sanityCheck.BuiltInType == BuiltInType.ExtensionObject && builtInType == BuiltInType.Null) ||
-                    (sanityCheck.BuiltInType == BuiltInType.Int32 && builtInType == BuiltInType.Enumeration) ||
-                    (sanityCheck.BuiltInType == BuiltInType.ByteString && builtInType == BuiltInType.Byte) ||
-                    (builtInType == BuiltInType.Variant));
+                (sanityCheck.BuiltInType == BuiltInType.ExtensionObject && builtInType == BuiltInType.Null) ||
+                (sanityCheck.BuiltInType == BuiltInType.Int32 && builtInType == BuiltInType.Enumeration) ||
+                (sanityCheck.BuiltInType == BuiltInType.ByteString && builtInType == BuiltInType.Byte) ||
+                (builtInType == BuiltInType.Variant));
 #endif																				 
         }
         #endregion

Stack/Opc.Ua.Core/Types/BuiltIn/NodeId.cs

@@ -10,6 +10,10 @@
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 */
 
+// define to enable checks for a null NodeId modification
+// some tests are failing with this enabled, only turn on to catch issues
+// #define IMMUTABLENULLNODEID
+
 using System;
 using System.Collections;
 using System.Collections.Generic;
@@ -866,8 +870,11 @@ internal static NodeId InternalParse(string text, bool namespaceSet)
         /// Returns an instance of a null NodeId.
         /// </summary>
         public static NodeId Null => s_Null;
-
+#if IMMUTABLENULLNODEID
+        private static readonly NodeId s_Null = new ImmutableNodeId();
+#else
         private static readonly NodeId s_Null = new NodeId();
+#endif
         #endregion
 
         #region Public Methods (and some Internals)
@@ -1001,6 +1008,7 @@ public static ExpandedNodeId ToExpandedNodeId(NodeId nodeId, NamespaceTable name
         /// </summary>
         internal void SetNamespaceIndex(ushort value)
         {
+            ValidateImmutableNodeIdIsNotModified();
             m_namespaceIndex = value;
         }
 
@@ -1009,6 +1017,7 @@ internal void SetNamespaceIndex(ushort value)
         /// </summary>
         internal void SetIdentifier(IdType idType, object value)
         {
+            ValidateImmutableNodeIdIsNotModified();
             m_identifierType = idType;
 
             switch (idType)
@@ -1038,6 +1047,8 @@ internal void SetIdentifier(IdType idType, object value)
         /// </summary>
         internal void SetIdentifier(string value, IdType idType)
         {
+            ValidateImmutableNodeIdIsNotModified();
+
             m_identifierType = idType;
             SetIdentifier(IdType.String, value);
         }
@@ -1495,6 +1506,8 @@ internal string IdentifierText
             }
             set
             {
+                ValidateImmutableNodeIdIsNotModified();
+
                 NodeId nodeId = NodeId.Parse(value);
 
                 m_namespaceIndex = nodeId.NamespaceIndex;
@@ -1859,6 +1872,21 @@ private static void FormatIdentifier(IFormatProvider formatProvider, StringBuild
                 }
             }
         }
+
+        /// <summary>
+        /// Validate that an immutable NodeId is not overwritten.
+        /// </summary>
+        /// <exception cref="InvalidOperationException"></exception>
+        [Conditional("IMMUTABLENULLNODEID")]
+        private void ValidateImmutableNodeIdIsNotModified()
+        {
+#if IMMUTABLENULLNODEID
+            if (this is ImmutableNodeId)
+            {
+                throw new InvalidOperationException("Cannot modify the immutable NodeId.Null.");
+            }
+#endif
+        }
         #endregion
 
         #region Private Fields
@@ -1868,6 +1896,20 @@ private static void FormatIdentifier(IFormatProvider formatProvider, StringBuild
         #endregion
     }
 
+#if IMMUTABLENULLNODEID
+    #region ImmutableNodeId
+    /// <summary>
+    /// A NodeId class as helper to catch if the immutable NodeId.Null is being modified.
+    /// </summary>
+    internal class ImmutableNodeId : NodeId
+    {
+        internal ImmutableNodeId()
+        {
+        }
+    }
+    #endregion
+#endif
+
     #region NodeIdCollection Class
     /// <summary>
     /// A collection of NodeIds.
@@ -1993,6 +2035,7 @@ public class NodeIdParsingOptions
         /// </summary>
         public ushort[] ServerMappings { get; set; }
     }
+
     #region NodeIdComparer Class
     /// <summary>
     /// Helper which implements a NodeId IEqualityComparer for Linq queries.

Tests/Opc.Ua.Core.Tests/Types/Encoders/EncoderCommon.cs

@@ -79,7 +79,6 @@ public class EncoderCommon
         protected BufferManager BufferManager { get; private set; }
         protected RecyclableMemoryStreamManager RecyclableMemoryManager { get; private set; }
 
-
         #region Test Setup
         [OneTimeSetUp]
         protected void OneTimeSetUp()
@@ -112,6 +111,8 @@ protected void SetUp()
         [TearDown]
         protected void TearDown()
         {
+            // ensure after every test that the Null NodeId was not modified
+            Assert.True(NodeId.Null.IsNullNodeId);
         }
 
         /// <summary>

Tests/Opc.Ua.Core.Tests/Types/Encoders/EncoderTests.cs

@@ -973,6 +973,30 @@ public void EncodeMatrixInArrayOverflow(
                 Assert.AreEqual((StatusCode)StatusCodes.BadEncodingLimitsExceeded, (StatusCode)sre.StatusCode, sre.Message);
             }
         }
+
+        /// <summary>
+        /// Test if deserializing an extensionObject alters the Null NodeId.
+        /// </summary>
+        /// <remarks>
+        /// Issue was raised in github #2974.
+        /// </remarks>
+        [Test]
+        public void EnsureNodeIdNullIsNotModified()
+        {
+            var text1 = "[{\"Body\":{\"KeyValuePair\":{\"@xmlns\":\"http://opcfoundation.org/UA/2008/02/Types.xsd\"," +
+                "\"Key\":{\"Name\":\"o\",\"NamespaceIndex\":\"0\"},\"Value\":{\"Value\":" +
+                "{\"ListOfExtensionObject\":{\"ExtensionObject\":[" +
+                "{\"Body\":{\"KeyValuePair\":{\"Key\":{\"Name\":\"stringProp\",\"NamespaceIndex\":\"0\"},\"Value\":{\"Value\":" +
+                "{\"String\":\"EinString\"}}}},\"TypeId\":{\"Identifier\":\"i=14801\"}},{\"Body\":{\"KeyValuePair\":{\"Key\":" +
+                "{\"Name\":\"intProp\",\"NamespaceIndex\":\"0\"},\"Value\":{\"Value\":{\"Int32\":\"1\"}}}},\"TypeId\":" +
+                "{\"Identifier\":\"i=14802\"}}]}}}}},\"TypeId\":" +
+                "{\"Identifier\":\"i=14803\"}}]";
+
+            JsonConvert.DeserializeObject<ExtensionObject[]>(text1);
+
+            Assert.NotNull(NodeId.Null);
+            Assert.True(NodeId.Null.IsNullNodeId);
+        }
         #endregion
 
         #region Private Methods