- Protect against buffer overwrite

- Assert array element size meets expectations - Use std::unique_ptr to avoid memory leak
ni · Jan 31, 2025 · 2381e43 · 2381e43
1 parent 44cf697
commit 2381e43
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 10 deletions.
diff --git a/src/lv_interop.h b/src/lv_interop.h
@@ -105,6 +105,7 @@ namespace grpc_labview
         T* bytes()
         {
             static_assert(!std::is_class<T>::value, "T must not be a struct/class type.");
+            static_assert(sizeof(T) <= 8, "Need to revisit logic if we ever have element size larger than 8 bytes.");
             return (T*)(rawBytes);
         }
     };

diff --git a/src/message_metadata.cc b/src/message_metadata.cc
@@ -9,7 +9,7 @@ namespace grpc_labview
     //---------------------------------------------------------------------
     // Functions for calculating whether an element is considered a well known type.
     //---------------------------------------------------------------------
-    static wellknown::Types IsWellKnownDouble2DArray(const MessageElementMetadata& metadata)
+    static wellknown::Types CalculateDouble2DArrayWellKnownTypeEnum(const MessageElementMetadata& metadata)
     {
         if (!metadata.isRepeated)
         {
@@ -22,7 +22,7 @@ namespace grpc_labview
     //---------------------------------------------------------------------
     std::map<const std::string, wellknown::Types(*)(const MessageElementMetadata&)> MessageElementMetadata::_wellKnownTypeFunctionMap =
     {
-        { wellknown::Double2DArray::GetMessageName(), IsWellKnownDouble2DArray}
+        { wellknown::Double2DArray::GetMessageName(), CalculateDouble2DArrayWellKnownTypeEnum}
     };
 
     //---------------------------------------------------------------------
@@ -85,7 +85,7 @@ namespace grpc_labview
         if (lvMetadata->elements != nullptr)
         {
             // byteAlignment for LVMessageElementMetadata would be the size of its largest element which is a LStrHandle
-            auto lvElement = (LVMessageElementMetadata*)(*lvMetadata->elements)->bytes(0, sizeof(LStrHandle));
+            auto lvElement = (LVMessageElementMetadata*)(*lvMetadata->elements)->bytes(0, alignof(LVMessageElementMetadata));
             auto metadataVersion = 1;
             InitializeElements(metadataOwner, lvElement, (*lvMetadata->elements)->cnt, metadataVersion);
         }
@@ -104,7 +104,7 @@ namespace grpc_labview
         if (lvMetadata->elements != nullptr)
         {
             // byteAlignment for LVMessageElementMetadata would be the size of its largest element which is a LStrHandle
-            auto lvElement = (LVMessageElementMetadata*)(*lvMetadata->elements)->bytes(0, sizeof(LStrHandle));
+            auto lvElement = (LVMessageElementMetadata*)(*lvMetadata->elements)->bytes(0, alignof(LVMessageElementMetadata));
             auto metadataVersion = 2;
             InitializeElements(metadataOwner, lvElement, (*lvMetadata->elements)->cnt, metadataVersion);
         }

diff --git a/src/well_known_types.cc b/src/well_known_types.cc
@@ -20,8 +20,8 @@ namespace grpc_labview
             if (_instance == nullptr)
             {
                 _instance = new MetadataOwner();
-                auto owner = _instance->_owner;
-                owner->RegisterMetadata(Double2DArray::GetMetadata(owner));
+                auto& owner = _instance->_owner;
+                owner->RegisterMetadata(Double2DArray::GetMetadata(owner.get()));
                 owner->FinalizeMetadata();
             }
             return *_instance;
@@ -31,7 +31,7 @@ namespace grpc_labview
         //---------------------------------------------------------------------
         MetadataOwner::MetadataOwner()
         {
-            _owner = new MessageElementMetadataOwner();
+            _owner = std::make_unique<MessageElementMetadataOwner>();
         }
 
         //---------------------------------------------------------------------
@@ -141,8 +141,12 @@ namespace grpc_labview
                 auto array = *(LV2DArrayHandle*)start;
                 (*array)->dimensionSizes[0] = rows;
                 (*array)->dimensionSizes[1] = columns;
-                auto byteCount = elementCount * sizeof(double);
-                memcpy((*array)->bytes<double>(), dataValue->_value.data(), byteCount);
+                // Protect against a malformed message where the amount of data sent doesn't match the dimension sizes.
+                // LV will automatically pad/handle writing less data than was allocated to the array so we just need
+                // to make sure we don't write more data than was allocated to the array.
+                auto lvByteCount = elementCount * sizeof(double);
+                auto protobufByteCount = dataValue->_value.size() * sizeof(double);
+                memcpy((*array)->bytes<double>(), dataValue->_value.data(), std::min(lvByteCount, protobufByteCount));
             }
         }
 

diff --git a/src/well_known_types.h b/src/well_known_types.h
@@ -42,7 +42,7 @@ namespace grpc_labview
         private:
             static MetadataOwner* _instance;
             static std::mutex _mutex;
-            MessageElementMetadataOwner* _owner;
+            std::unique_ptr<MessageElementMetadataOwner> _owner;
 
             MetadataOwner();
         };