Fix memory issues in the uacpp backend pointed out by asan

The backend leaked extension objects and node ids when writing.
An unsuccessful call to write lead to a crash caused by accessing
an invalid pointer.
Allocating uacpp objects with new while the OPC UA stack uses
free() when clearing the data caused asan to abort the program
due to an alloc_dealloc_mismatch.

Change-Id: Ib6e5b2347f029c21f4d5ed6771362bc77f39211d
Reviewed-by: Maurice Kalinowski <maurice.kalinowski@qt.io>

2 files changed, 18 insertions, 17 deletions

diff --git a/src/plugins/opcua/uacpp/quacppbackend.cpp b/src/plugins/opcua/uacpp/quacppbackend.cpp
index 098781d..694ead1 100644
--- a/src/plugins/opcua/uacpp/quacppbackend.cpp
+++ b/src/plugins/opcua/uacpp/quacppbackend.cpp
@@ -216,13 +216,12 @@ void UACppAsyncBackend::readAttributes(uintptr_t handle, const UaNodeId &id, QOp
 
     QVector<QOpcUaReadResult> vec;
 
-    OpcUa_NodeId *nativeId = id.copy();
     int attributeSize = 0;
 
     qt_forEachAttribute(attr, [&](QOpcUa::NodeAttribute attribute){
         attributeSize++;
         nodeToRead.resize(attributeSize);
-        OpcUa_NodeId_CopyTo(nativeId, &nodeToRead[attributeSize - 1].NodeId);
+        id.copyTo(&nodeToRead[attributeSize - 1].NodeId);
         nodeToRead[attributeSize - 1].AttributeId = toUaAttributeId(attribute);
         if (indexRange.size()) {
             UaString ir(indexRange.toUtf8().constData());
@@ -275,7 +274,8 @@ void UACppAsyncBackend::writeAttribute(uintptr_t handle, const UaNodeId &id, QOp
     if (result.isBad())
         qCWarning(QT_OPCUA_PLUGINS_UACPP) << "Writing attribute failed:" << result.toString().toUtf8();
 
-    emit attributeWritten(handle, attrId, result.isGood() ? value : QVariant(), static_cast<QOpcUa::UaStatusCode>(writeResults[0]));
+    emit attributeWritten(handle, attrId, result.isGood() ? value : QVariant(), writeResults.length() ?
+                              static_cast<QOpcUa::UaStatusCode>(writeResults[0]) : static_cast<QOpcUa::UaStatusCode>(result.statusCode()));
 }
 
 void UACppAsyncBackend::writeAttributes(uintptr_t handle, const UaNodeId &id, QOpcUaNode::AttributeMap toWrite, QOpcUa::Types /*valueAttributeType*/)
diff --git a/src/plugins/opcua/uacpp/quacppvalueconverter.cpp b/src/plugins/opcua/uacpp/quacppvalueconverter.cpp
index b9b82ff..cd2bcd6 100644
--- a/src/plugins/opcua/uacpp/quacppvalueconverter.cpp
+++ b/src/plugins/opcua/uacpp/quacppvalueconverter.cpp
@@ -509,16 +509,13 @@ void scalarFromQVariant<OpcUa_Guid, QUuid>(const QVariant &var, OpcUa_Guid *ptr)
 
 void createExtensionObject(QByteArray &data, QOpcUaBinaryDataEncoding::TypeEncodingId id, OpcUa_ExtensionObject *ptr)
 {
-    OpcUa_ExtensionObject *opcuaObj;
-    OpcUa_ExtensionObject_Create(&opcuaObj);
-    opcuaObj->Encoding = OpcUa_ExtensionObjectEncoding_Binary;
-    opcuaObj->Body.Binary.Data = reinterpret_cast<OpcUa_Byte *>(data.data());
-    opcuaObj->Body.Binary.Length = data.size();
-    opcuaObj->BodySize = data.length();
+    OpcUa_ExtensionObject_Initialize(ptr);
+    UaByteArray arr(data.data(), data.length());
+    arr.copyTo(&ptr->Body.Binary);
+    ptr->Encoding = OpcUa_ExtensionObjectEncoding_Binary;
+    ptr->BodySize = data.length();
     const UaNodeId temp(static_cast<OpcUa_UInt32>(id));
-    temp.copyTo(&opcuaObj->TypeId.NodeId);
-    OpcUa_ExtensionObject_CopyTo(opcuaObj, ptr);
-    //OpcUa_ExtensionObject_Clear(opcuaObj);
+    temp.copyTo(&ptr->TypeId.NodeId);
 }
 
 template<>
@@ -589,8 +586,9 @@ OpcUa_Variant arrayFromQVariant(const QVariant &var, const OpcUa_BuiltInType typ
         opcuavariant.Datatype = type;
         opcuavariant.ArrayType = OpcUa_True;
         opcuavariant.Value.Array.Length = list.size();
-        TARGETTYPE *arr = new TARGETTYPE[list.size()];
-        opcuavariant.Value.Array.Value.Array = static_cast<OpcUa_Void *>(arr);
+        // Use malloc() instead of new because the OPC UA stack uses free() internally when clearing the data
+        TARGETTYPE *arr = static_cast<TARGETTYPE *>(malloc(sizeof(TARGETTYPE) * list.size()));
+        opcuavariant.Value.Array.Value.Array = arr;
 
         for (int i = 0; i < list.size(); ++i)
             scalarFromQVariant<TARGETTYPE, QTTYPE>(list[i], &arr[i]);
@@ -598,7 +596,7 @@ OpcUa_Variant arrayFromQVariant(const QVariant &var, const OpcUa_BuiltInType typ
         return opcuavariant;
     }
 
-    TARGETTYPE *temp = (TARGETTYPE*)&opcuavariant.Value;
+    TARGETTYPE *temp = reinterpret_cast<TARGETTYPE *>(&opcuavariant.Value);
     scalarFromQVariant<TARGETTYPE, QTTYPE>(var, temp);
     opcuavariant.Datatype = type;
     return opcuavariant;
@@ -621,7 +619,9 @@ OpcUa_Variant arrayFromQVariantPointer(const QVariant &var, const OpcUa_BuiltInT
         opcuavariant.Datatype = type;
         opcuavariant.ArrayType = OpcUa_True;
         opcuavariant.Value.Array.Length = list.size();
-        TARGETTYPE *arr = new TARGETTYPE[list.size()];
+        // Use malloc() instead of new because the OPC UA stack uses free() internally when clearing the data
+        TARGETTYPE *arr = static_cast<TARGETTYPE *>(malloc(sizeof(TARGETTYPE) * list.size()));
+
         opcuavariant.Value.Array.Value.Array = arr;
 
         for (int i = 0; i < list.size(); ++i)
@@ -633,7 +633,8 @@ OpcUa_Variant arrayFromQVariantPointer(const QVariant &var, const OpcUa_BuiltInT
     // Taking one pointer for all as it is union
     TARGETTYPE **temp = reinterpret_cast<TARGETTYPE **>(&opcuavariant.Value.Guid);
     // We have to allocate, otherwise copyTo() will not do any action
-    *temp = new TARGETTYPE;
+    // Use malloc() instead of new because the OPC UA stack uses free() internally when clearing the data
+    *temp = static_cast<TARGETTYPE *>(malloc(sizeof(TARGETTYPE)));
     scalarFromQVariant<TARGETTYPE, QTTYPE>(var, *temp);
     opcuavariant.Datatype = type;
     return opcuavariant;