fix(server): Fix a double-free after adjusting a value type

jpfr · Sep 11, 2024 · f102faa · f102faa
1 parent 1c9116d
commit f102faa
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 3 deletions.
diff --git a/src/client/ua_client_highlevel.c b/src/client/ua_client_highlevel.c
@@ -880,7 +880,12 @@ AttributeReadCallback(UA_Client *client, void *userdata,
 
     /* Check the type. Try to adjust "in situ" if no match. */
     if(!UA_Variant_hasScalarType(&dv->value, ctx->resultType)) {
+        /* Remember the old pointer, adjustType can "unwrap" a type but won't
+         * free the wrapper. Because the server code still keeps the wrapper. */
+        void *oldVal = dv->value.data;
         adjustType(&dv->value, ctx->resultType);
+        if(dv->value.data != oldVal)
+            UA_free(oldVal);
         if(!UA_Variant_hasScalarType(&dv->value, ctx->resultType)) {
             res = UA_STATUSCODE_BADINTERNALERROR;
             goto finish;

diff --git a/src/ua_util.c b/src/ua_util.c
@@ -42,8 +42,6 @@ adjustType(UA_Variant *value, const UA_DataType *targetType) {
         value->type = &UA_TYPES[UA_TYPES_BYTE];
         value->arrayLength = str->length;
         value->data = str->data;
-        if(value->storageType != UA_VARIANT_DATA_NODELETE)
-            UA_free(str);
         return;
     }
 

diff --git a/src/ua_util_internal.h b/src/ua_util_internal.h
@@ -28,7 +28,12 @@ _UA_BEGIN_DECLS
 #define UA_MACRO_EXPAND(x) x
 
 /* Try if the type of the value can be adjusted "in situ" to the target type.
- * That can be done, for example, to map between int32 and an enum. */
+ * That can be done, for example, to map between int32 and an enum.
+ *
+ * This can also "unwrap" a type. For example: string -> array of bytes
+ *
+ * If value->data is changed during adjustType, free the pointer afterwards (if
+ * you did not keep the original variant for _clear). */
 void
 adjustType(UA_Variant *value, const UA_DataType *targetType);
 

diff --git a/tests/server/check_services_attributes.c b/tests/server/check_services_attributes.c
@@ -894,6 +894,58 @@ START_TEST(WriteSingleAttributeValueEnum) {
     UA_DataValue_clear(&resp);
 } END_TEST
 
+/* Writing a ByteString into a byte array */
+START_TEST(WriteSingleAttributeStringToByteArray) {
+    UA_WriteValue wValue;
+    UA_WriteValue_init(&wValue);
+
+    UA_VariableAttributes vattr = UA_VariableAttributes_default;
+    UA_Byte testArray[4] = {1,2,3,4};
+    UA_UInt32 testArrayDims[1] = {4};
+    UA_Variant_setArray(&vattr.value, testArray, 4, &UA_TYPES[UA_TYPES_BYTE]);
+    vattr.value.arrayDimensions = testArrayDims;
+    vattr.value.arrayDimensionsSize = 1;
+    vattr.description = UA_LOCALIZEDTEXT("locale","test array");
+    vattr.displayName = UA_LOCALIZEDTEXT("locale","test array");
+    vattr.valueRank = UA_VALUERANK_ONE_DIMENSION;
+    vattr.arrayDimensions = testArrayDims;
+    vattr.arrayDimensionsSize = 1;
+    vattr.dataType = UA_TYPES[UA_TYPES_BYTE].typeId;
+    UA_QualifiedName arrayName = UA_QUALIFIEDNAME(1, "test array");
+    UA_NodeId arrayNodeId = UA_NODEID_STRING(1, "test.array");
+    UA_NodeId parentNodeId = UA_NODEID_NUMERIC(0, UA_NS0ID_OBJECTSFOLDER);
+    UA_NodeId parentReferenceNodeId = UA_NODEID_NUMERIC(0, UA_NS0ID_ORGANIZES);
+    UA_StatusCode retval =
+        UA_Server_addVariableNode(server, arrayNodeId, parentNodeId,
+                                  parentReferenceNodeId, arrayName,
+                                  UA_NODEID_NUMERIC(0, UA_NS0ID_BASEDATAVARIABLETYPE),
+                                  vattr, NULL, NULL);
+    ck_assert_int_eq(retval, UA_STATUSCODE_GOOD);
+
+    UA_String testString = UA_STRING("open");
+    UA_Variant_setScalar(&wValue.value.value, &testString, &UA_TYPES[UA_TYPES_BYTESTRING]);
+    wValue.value.hasValue = true;
+    wValue.nodeId = UA_NODEID_STRING(1, "test.array");
+    wValue.attributeId = UA_ATTRIBUTEID_VALUE;
+    retval = UA_Server_write(server, &wValue);
+    ck_assert_int_eq(retval, UA_STATUSCODE_GOOD);
+
+    UA_ReadValueId rvi;
+    UA_ReadValueId_init(&rvi);
+    rvi.nodeId = UA_NODEID_STRING(1, "test.array");
+    rvi.attributeId = UA_ATTRIBUTEID_VALUE;
+    UA_DataValue resp = UA_Server_read(server, &rvi, UA_TIMESTAMPSTORETURN_NEITHER);
+    ck_assert_int_eq(resp.status, UA_STATUSCODE_GOOD);
+    ck_assert(resp.hasValue);
+    ck_assert(UA_Variant_hasArrayType(&resp.value, &UA_TYPES[UA_TYPES_BYTE]));
+
+    UA_Byte *arr = (UA_Byte*)resp.value.data;
+    arr[0] = 'o';
+    arr[1] = 'p';
+
+    UA_DataValue_clear(&resp);
+} END_TEST
+
 START_TEST(WriteSingleAttributeValueRangeFromScalar) {
     UA_WriteValue wValue;
     UA_WriteValue_init(&wValue);
@@ -1179,6 +1231,7 @@ static Suite * testSuite_services_attributes(void) {
     tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeValue);
     tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeValueWithServerTimestamp);
     tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeValueEnum);
+    tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeStringToByteArray);
     tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeDataType);
     tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeValueRangeFromScalar);
     tcase_add_test(tc_writeSingleAttributes, WriteSingleAttributeValueRangeFromArray);