BUG: Fix reading of volume property (.vp) file

Color transfer function line in .vp files can be several thousands character long when there are many control points but the reader had a hardcoded limit at 1024 bytes. Fixed by switching to use an API that does not limit line length. Fixes the issue reported at https://discourse.slicer.org/t/saving-volume-properties/36475/12
lassoan · May 31, 2024 · 91e0874 · 91e0874
1 parent 9c48097
commit 91e0874
Show file tree

Hide file tree

Showing 7 changed files with 46 additions and 54 deletions.
diff --git a/Libs/MRML/Core/vtkMRMLColorTableStorageNode.cxx b/Libs/MRML/Core/vtkMRMLColorTableStorageNode.cxx
@@ -91,20 +91,20 @@ int vtkMRMLColorTableStorageNode::ReadDataInternal(vtkMRMLNode *refNode)
 
     colorNode->NamesInitialisedOff();
 
-    char line[1024];
+    std::string line;
     // save the valid lines in a vector, parse them once know the max id
     std::vector<std::string>lines;
     int maxID = 0;
     while (fstr.good())
     {
-      fstr.getline(line, 1024);
+      std::getline(fstr, line);
 
       // does it start with a #?
       if (line[0] == '#')
       {
         vtkDebugMacro("Comment line, skipping:\n\"" << line << "\"");
         // sanity check: does the procedural header match?
-        if (strncmp(line, "# Color procedural file", 23) == 0)
+        if (line.compare(0, 23, "# Color procedural file") == 0)
         {
           vtkErrorMacro("ReadDataInternal:\nfound a comment that this file "
                         << " is a procedural color file, returning:\n"

diff --git a/Libs/MRML/Core/vtkMRMLProceduralColorStorageNode.cxx b/Libs/MRML/Core/vtkMRMLProceduralColorStorageNode.cxx
@@ -96,11 +96,11 @@ int vtkMRMLProceduralColorStorageNode::ReadDataInternal(vtkMRMLNode *refNode)
     colorNode->NamesInitialisedOff();
     ctf->RemoveAllPoints();
 
-    char line[1024];
+    std::string line;
 
     while (fstr.good())
     {
-      fstr.getline(line, 1024);
+      std::getline(fstr, line);
 
       // does it start with a #?
       if (line[0] == '#')

diff --git a/Libs/MRML/Logic/vtkMRMLSliceLogic.cxx b/Libs/MRML/Logic/vtkMRMLSliceLogic.cxx
@@ -1405,7 +1405,7 @@ void vtkMRMLSliceLogic::CreateSliceModel()
   // to happen after they have been set, so do it every event for now
   if ( this->SliceModelNode != nullptr )
   {
-    char description[256];
+    std::string description;
     std::stringstream ssD;
     if (this->SliceNode && this->SliceNode->GetID() )
     {
@@ -1416,8 +1416,8 @@ void vtkMRMLSliceLogic::CreateSliceModel()
       ssD << " CompositeID " << this->SliceCompositeNode->GetID();
     }
 
-    ssD.getline(description,256);
-    this->SliceModelNode->SetDescription(description);
+    std::getline(ssD, description);
+    this->SliceModelNode->SetDescription(description.c_str());
   }
 }
 

diff --git a/Modules/Loadable/Markups/Logic/vtkSlicerMarkupsLogic.cxx b/Modules/Loadable/Markups/Logic/vtkSlicerMarkupsLogic.cxx
@@ -1874,7 +1874,8 @@ void vtkSlicerMarkupsLogic::RenameAllControlPointsFromCurrentFormat(vtkMRMLMarku
   // get the format string with the list name replaced
   std::string formatString = markupsNode->ReplaceListNameInControlPointLabelFormat();
   bool numberInFormat = false;
-  std::vector<char> buffVector(vtkMRMLMarkupsFiducialStorageNode::GetMaximumLineLength());
+  const int maxLineLength = 1024;
+  std::vector<char> buffVector(maxLineLength);
   char* buff = &(buffVector[0]);
   if (formatString.find("%d") != std::string::npos ||
       formatString.find("%g") != std::string::npos ||
@@ -1920,18 +1921,18 @@ void vtkSlicerMarkupsLogic::RenameAllControlPointsFromCurrentFormat(vtkMRMLMarku
         if (formatString.find("%d") != std::string::npos)
         {
           // integer
-          sprintf(buff,formatString.c_str(),atoi(oldNumber.c_str()));
+          snprintf(buff, maxLineLength, formatString.c_str(),atoi(oldNumber.c_str()));
         }
         else
         {
           // float
-          sprintf(buff,formatString.c_str(),atof(oldNumber.c_str()));
+          snprintf(buff, maxLineLength, formatString.c_str(),atof(oldNumber.c_str()));
         }
       }
       else
       {
         // no number found, use n
-        sprintf(buff,formatString.c_str(),n);
+        snprintf(buff, maxLineLength, formatString.c_str(),n);
       }
       markupsNode->SetNthControlPointLabel(n, std::string(buff));
     }

diff --git a/Modules/Loadable/Markups/MRML/vtkMRMLMarkupsFiducialStorageNode.cxx b/Modules/Loadable/Markups/MRML/vtkMRMLMarkupsFiducialStorageNode.cxx
@@ -551,8 +551,7 @@ int vtkMRMLMarkupsFiducialStorageNode::ReadDataInternal(vtkMRMLNode *refNode)
       markupsNode->RemoveAllControlPoints();
     }
 
-    std::vector<char> lineBuff(vtkMRMLMarkupsFiducialStorageNode::GetMaximumLineLength());
-    char* line = &(lineBuff[0]);
+    std::string line;
 
     // save the valid lines in a vector, parse them once know the max id
     std::vector<std::string>lines;
@@ -565,7 +564,7 @@ int vtkMRMLMarkupsFiducialStorageNode::ReadDataInternal(vtkMRMLNode *refNode)
 
     while (fstr.good())
     {
-      fstr.getline(line, vtkMRMLMarkupsFiducialStorageNode::GetMaximumLineLength());
+      std::getline(fstr, line);
 
       // does it start with a #?
       if (line[0] == '#')
@@ -696,7 +695,7 @@ int vtkMRMLMarkupsFiducialStorageNode::ReadDataInternal(vtkMRMLNode *refNode)
           else
           {
             vtkDebugMacro("\n\n\n\nVersion = " << version << ", got a line: \n\"" << line << "\"");
-            this->SetPointFromString(markupsNode, markupsNode->GetNumberOfControlPoints(), line);
+            this->SetPointFromString(markupsNode, markupsNode->GetNumberOfControlPoints(), line.c_str());
 
             thisMarkupNumber++;
           } // point line

diff --git a/Modules/Loadable/Markups/MRML/vtkMRMLMarkupsFiducialStorageNode.h b/Modules/Loadable/Markups/MRML/vtkMRMLMarkupsFiducialStorageNode.h
@@ -74,6 +74,8 @@ class VTK_SLICER_MARKUPS_MODULE_MRML_EXPORT vtkMRMLMarkupsFiducialStorageNode :
   std::string ConvertStringFromStorageFormat(std::string input);
 
   /// Buffer size for parsing files during read.
+  /// This method is deprecated, as it is not used anymore, and it will be removed
+  /// in future software versions.
   static int GetMaximumLineLength() { return 1024; }
 
 protected:

diff --git a/Modules/Loadable/VolumeRendering/MRML/vtkMRMLVolumePropertyStorageNode.cxx b/Modules/Loadable/VolumeRendering/MRML/vtkMRMLVolumePropertyStorageNode.cxx
@@ -75,96 +75,86 @@ int vtkMRMLVolumePropertyStorageNode::ReadDataInternal(vtkMRMLNode *refNode)
     vtkErrorMacro("Cannot open volume property file: " << fullName);
     return 0;
   }
-  char line[1024];
-  std::string sline;
+  std::string line;
 
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     int value;
     std::stringstream ss;
-    ss << sline;
+    ss << line;
     ss >> value;
     vpNode->GetVolumeProperty()->SetInterpolationType(value);
   }
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     int value;
     std::stringstream ss;
-    ss << sline;
+    ss << line;
     ss >> value;
     vpNode->GetVolumeProperty()->SetShade(value);
   }
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     double value;
     std::stringstream ss;
-    ss << sline;
+    ss << line;
     ss >> value;
     vpNode->GetVolumeProperty()->SetDiffuse(value);
   }
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     double value;
     std::stringstream ss;
-    ss << sline;
+    ss << line;
     ss >> value;
     vpNode->GetVolumeProperty()->SetAmbient(value);
   }
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     double value;
     std::stringstream ss;
-    ss << sline;
+    ss << line;
     ss >> value;
     vpNode->GetVolumeProperty()->SetSpecular(value);
   }
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     double value;
     std::stringstream ss;
-    ss << sline;
+    ss << line;
     ss >> value;
     vpNode->GetVolumeProperty()->SetSpecularPower(value);
   }
 
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     vtkPiecewiseFunction *scalarOpacity=vtkPiecewiseFunction::New();
-    vpNode->GetPiecewiseFunctionFromString(sline, scalarOpacity),
+    vpNode->GetPiecewiseFunctionFromString(line, scalarOpacity),
     vpNode->SetScalarOpacity(scalarOpacity);
     scalarOpacity->Delete();
   }
 
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     vtkPiecewiseFunction *gradientOpacity=vtkPiecewiseFunction::New();
-    vpNode->GetPiecewiseFunctionFromString(sline, gradientOpacity);
+    vpNode->GetPiecewiseFunctionFromString(line, gradientOpacity);
     vpNode->SetGradientOpacity(gradientOpacity);
     gradientOpacity->Delete();
   }
 
-  ifs.getline(line, 1024);
-  sline = line;
-  if (!sline.empty())
+  std::getline(ifs, line);
+  if (!line.empty())
   {
     vtkColorTransferFunction *colorTransfer=vtkColorTransferFunction::New();
-    vpNode->GetColorTransferFunctionFromString(sline, colorTransfer);
+    vpNode->GetColorTransferFunctionFromString(line, colorTransfer);
     vpNode->SetColor(colorTransfer);
     colorTransfer->Delete();
   }