Repository: Kitware/kwiver (language: python)

Issue:
Potential memory leak in SystemTools
The [DuplicateString](https://github.com/Kitware/kwiver/blob/f38e71e31e6b8d6d0b0fcde2c5ef58aae8b2bd35/vital/kwiversys/SystemTools.cxx#L1797-L1805) method allocates a char array that could leak. It appears to be unused except in the AppendStrings methods which are also unused. I suggest these methods be removed.


Discussion / hints:

Fix patch:
diff --git a/vital/kwiversys/SystemTools.cxx b/vital/kwiversys/SystemTools.cxx
index da34eb943f..3fc741d557 100644
--- a/vital/kwiversys/SystemTools.cxx
+++ b/vital/kwiversys/SystemTools.cxx
@@ -1563,55 +1563,6 @@ std::string SystemTools::AddSpaceBetweenCapitalizedWords(
   return n;
 }
 
-char* SystemTools::AppendStrings(const char* str1, const char* str2)
-{
-  if (!str1)
-    {
-    return SystemTools::DuplicateString(str2);
-    }
-  if (!str2)
-    {
-    return SystemTools::DuplicateString(str1);
-    }
-  size_t len1 = strlen(str1);
-  char *newstr = new char[len1 + strlen(str2) + 1];
-  if (!newstr)
-    {
-    return 0;
-    }
-  strcpy(newstr, str1);
-  strcat(newstr + len1, str2);
-  return newstr;
-}
-
-char* SystemTools::AppendStrings(
-  const char* str1, const char* str2, const char* str3)
-{
-  if (!str1)
-    {
-    return SystemTools::AppendStrings(str2, str3);
-    }
-  if (!str2)
-    {
-    return SystemTools::AppendStrings(str1, str3);
-    }
-  if (!str3)
-    {
-    return SystemTools::AppendStrings(str1, str2);
-    }
-
-  size_t len1 = strlen(str1), len2 = strlen(str2);
-  char *newstr = new char[len1 + len2 + strlen(str3) + 1];
-  if (!newstr)
-    {
-    return 0;
-    }
-  strcpy(newstr, str1);
-  strcat(newstr + len1, str2);
-  strcat(newstr + len1 + len2, str3);
-  return newstr;
-}
-
 // Return a lower case string
 std::string SystemTools::LowerCase(const std::string& s)
 {
@@ -1793,17 +1744,6 @@ const char* SystemTools::FindLastString(const char* str1, const char* str2)
   return NULL;
 }
 
-// Duplicate string
-char* SystemTools::DuplicateString(const char* str)
-{
-  if (str)
-    {
-    char *newstr = new char [strlen(str) + 1];
-    return strcpy(newstr, str);
-    }
-  return NULL;
-}
-
 // Return a cropped string
 std::string SystemTools::CropString(const std::string& s,
                                           size_t max_len)
diff --git a/vital/kwiversys/SystemTools.hxx.in b/vital/kwiversys/SystemTools.hxx.in
index bba5a5cd3b..818c986af3 100644
--- a/vital/kwiversys/SystemTools.hxx.in
+++ b/vital/kwiversys/SystemTools.hxx.in
@@ -188,13 +188,6 @@ public:
    */
   static const char* FindLastString(const char* str1, const char* str2);
 
-  /**
-   * Make a duplicate of the string similar to the strdup C function
-   * but use new to create the 'new' string, so one can use
-   * 'delete' to remove it. Returns 0 if the input is empty.
-   */
-  static char* DuplicateString(const char* str);
-
   /**
    * Return the string cropped to a given length by removing chars in the
    * center of the string and replacing them with an ellipsis (...)
@@ -235,17 +228,6 @@ public:
   static std::string AddSpaceBetweenCapitalizedWords(
     const std::string&);
 
-  /**
-   * Append two or more strings and produce new one.
-   * Programmer must 'delete []' the resulting string, which was allocated
-   * with 'new'.
-   * Return 0 if inputs are empty or there was an error
-   */
-  static char* AppendStrings(
-    const char* str1, const char* str2);
-  static char* AppendStrings(
-    const char* str1, const char* str2, const char* str3);
-
   /**
    * Estimate the length of the string that will be produced
    * from printing the given format string and arguments.  The


Test patch:
diff --git a/vital/kwiversys/testSystemTools.cxx b/vital/kwiversys/testSystemTools.cxx
index a0f904f721..67d8d1af08 100644
--- a/vital/kwiversys/testSystemTools.cxx
+++ b/vital/kwiversys/testSystemTools.cxx
@@ -434,28 +434,6 @@ static bool CheckStringOperations()
     res = false;
     }
 
-  char * cres =
-    kwsys::SystemTools::AppendStrings("Mary Had A"," Little Lamb.");
-  if (strcmp(cres,"Mary Had A Little Lamb."))
-    {
-    std::cerr
-      << "Problem with AppendStrings "
-      << "\"Mary Had A\" \" Little Lamb.\"" << std::endl;
-    res = false;
-    }
-  delete [] cres;
-
-  cres =
-    kwsys::SystemTools::AppendStrings("Mary Had"," A ","Little Lamb.");
-  if (strcmp(cres,"Mary Had A Little Lamb."))
-    {
-    std::cerr
-      << "Problem with AppendStrings "
-      << "\"Mary Had\" \" A \" \"Little Lamb.\"" << std::endl;
-    res = false;
-    }
-  delete [] cres;
-
   if (kwsys::SystemTools::CountChar("Mary Had A Little Lamb.",'a') != 3)
     {
     std::cerr
@@ -516,16 +494,6 @@ static bool CheckStringOperations()
     res = false;
     }
 
-  cres = kwsys::SystemTools::DuplicateString("Mary Had A Little Lamb.");
-  if (strcmp(cres,"Mary Had A Little Lamb."))
-    {
-    std::cerr
-      << "Problem with DuplicateString "
-      << "\"Mary Had A Little Lamb.\"" << std::endl;
-    res = false;
-    }
-  delete [] cres;
-
   test = "Mary Had A Little Lamb.";
   if (kwsys::SystemTools::CropString(test,13) !=
       "Mary ...Lamb.")