From 1c395c7486bccfd1fadb5a4f8f719aebda8b74f3 Mon Sep 17 00:00:00 2001
From: Sean McBride <sean@rogue-research.com>
Date: Mon, 10 Jul 2023 17:09:57 -0400
Subject: [PATCH] Fixed out of bounds array access with empty arrays

It was incorrect to assume there was always a 0th element.  Instead, check that the array is not empty first.

Added new test cases to test empty arrays.

Thanks to David Gobbi for noticing this bug during review of something else.
---
 Common/Core/Testing/Cxx/TestVariant.cxx      | 39 ++++++++++++++++++++
 Common/Core/Testing/Cxx/TestVariantArray.cxx |  2 +-
 Common/Core/vtkVariant.cxx                   | 19 +++++++---
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/Common/Core/Testing/Cxx/TestVariant.cxx b/Common/Core/Testing/Cxx/TestVariant.cxx
index f5b191d8992..77ba4fd8ce3 100644
--- a/Common/Core/Testing/Cxx/TestVariant.cxx
+++ b/Common/Core/Testing/Cxx/TestVariant.cxx
@@ -18,7 +18,10 @@
   the U.S. Government retains certain rights in this software.
 -------------------------------------------------------------------------*/
 
+#include "vtkFloatArray.h"
+#include "vtkStringArray.h"
 #include "vtkVariant.h"
+#include "vtkVariantArray.h"
 
 int TestVariant(int, char*[])
 {
@@ -181,5 +184,41 @@ int TestVariant(int, char*[])
     errors++;
   }
 
+  // Regression test: ensure that empty arrays (of the 3 types) survive conversion to numeric.
+  // There used to be an incorrect assumption that arrays always had a 0th element.
+  {
+    vtkNew<vtkFloatArray> emptyArray;
+    vtkVariant arrayVariant(emptyArray);
+    bool isValid = true;
+    short numericValue = arrayVariant.ToShort(&isValid);
+    if (isValid || (numericValue != 0))
+    {
+      cerr << "empty vtkFloatArray should have failed to convert to numeric.\n";
+      errors++;
+    }
+  }
+  {
+    vtkNew<vtkStringArray> emptyArray;
+    vtkVariant arrayVariant(emptyArray);
+    bool isValid = true;
+    int numericValue = arrayVariant.ToInt(&isValid);
+    if (isValid || (numericValue != 0))
+    {
+      cerr << "empty vtkStringArray should have failed to convert to numeric.\n";
+      errors++;
+    }
+  }
+  {
+    vtkNew<vtkVariantArray> emptyArray;
+    vtkVariant arrayVariant(emptyArray);
+    bool isValid = true;
+    char numericValue = arrayVariant.ToChar(&isValid);
+    if (isValid || (numericValue != 0))
+    {
+      cerr << "empty vtkVariantArray should have failed to convert to numeric.\n";
+      errors++;
+    }
+  }
+
   return errors;
 }
diff --git a/Common/Core/Testing/Cxx/TestVariantArray.cxx b/Common/Core/Testing/Cxx/TestVariantArray.cxx
index 5c6d5cfbebc..af9cfc5dfd8 100644
--- a/Common/Core/Testing/Cxx/TestVariantArray.cxx
+++ b/Common/Core/Testing/Cxx/TestVariantArray.cxx
@@ -125,7 +125,6 @@ int TestVariantArray(int, char*[])
   double prob = 1.0 - 1.0 / size;
 
   vtkVariantArray* arr = vtkVariantArray::New();
-  vector<double> vec;
 
   // Resizing
   // * vtkTypeBool Allocate(vtkIdType sz);
@@ -226,6 +225,7 @@ int TestVariantArray(int, char*[])
   cerr << "Performing insert operations." << endl;
   vtkIdType id = 0;
   bool empty = true;
+  vector<double> vec;
   while (empty || vtkMath::Random() < prob)
   {
     empty = false;
diff --git a/Common/Core/vtkVariant.cxx b/Common/Core/vtkVariant.cxx
index bc78eea636f..de51af75d64 100644
--- a/Common/Core/vtkVariant.cxx
+++ b/Common/Core/vtkVariant.cxx
@@ -940,20 +940,29 @@ T vtkVariant::ToNumeric(bool* valid, T* vtkNotUsed(ignored)) const
       //       We convert the first value to double, then
       //       cast it back to the appropriate numeric type.
       vtkDataArray* da = vtkDataArray::SafeDownCast(this->Data.VTKObject);
-      return static_cast<T>(da->GetTuple1(0));
+      if (da->GetNumberOfTuples() > 0)
+      {
+        return static_cast<T>(da->GetTuple1(0));
+      }
     }
-    if (this->Data.VTKObject->IsA("vtkVariantArray"))
+    else if (this->Data.VTKObject->IsA("vtkVariantArray"))
     {
       // Note: This are not the best conversion.
       //       We convert the first value to double, then
       //       cast it back to the appropriate numeric type.
       vtkVariantArray* va = vtkVariantArray::SafeDownCast(this->Data.VTKObject);
-      return static_cast<T>(va->GetValue(0).ToDouble());
+      if (va->GetNumberOfValues() > 0)
+      {
+        return static_cast<T>(va->GetValue(0).ToDouble());
+      }
     }
-    if (this->Data.VTKObject->IsA("vtkStringArray"))
+    else if (this->Data.VTKObject->IsA("vtkStringArray"))
     {
       vtkStringArray* sa = vtkStringArray::SafeDownCast(this->Data.VTKObject);
-      return vtkVariantStringToNumeric<T>(sa->GetValue(0), valid);
+      if (sa->GetNumberOfValues() > 0)
+      {
+        return vtkVariantStringToNumeric<T>(sa->GetValue(0), valid);
+      }
     }
   }
   if (valid)
-- 
GitLab