Fix potential string iterator issue.

Added checks to StrCmpNum() function in common/sting.cpp to prevent
iterating past the end of the sting.  Also removed length of string
parameter since it did not seem to be used anywhere.

Fixes lp:1818157

https://bugs.launchpad.net/kicad/+bug/1818157

common/displlst.cpp

@@ -250,7 +250,7 @@ static int wxCALLBACK myCompareFunction( wxIntPtr aItem1, wxIntPtr aItem2,
     wxString* component1Name = (wxString*) aItem1;
     wxString* component2Name = (wxString*) aItem2;
 
-    return StrNumCmp( *component1Name, *component2Name, INT_MAX, true );
+    return StrNumCmp( *component1Name, *component2Name, true );
 }
 
 

common/lib_tree_model.cpp

@@ -77,7 +77,7 @@ void LIB_TREE_NODE::AssignIntrinsicRanks( bool presorted )
 
         std::sort( sort_buf.begin(), sort_buf.end(),
                 []( LIB_TREE_NODE* a, LIB_TREE_NODE* b ) -> bool
-                    { return StrNumCmp( a->Name, b->Name, INT_MAX, true ) > 0; } );
+                    { return StrNumCmp( a->Name, b->Name, true ) > 0; } );
 
         for( int i = 0; i < (int) sort_buf.size(); ++i )
             sort_buf[i]->IntrinsicRank = i;

common/string.cpp

@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2004-2017 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 2004-2019 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -39,13 +39,7 @@
  */
 static const char illegalFileNameChars[] = "\\/:\"<>|";
 
-/**
+
- * These Escape/Unescape routines use HTML-entity-reference-style encoding to handle
- * characters which are:
- *   (a) not legal in filenames
- *   (b) used as control characters in LIB_IDs
- *   (c) used to delineate hierarchical paths
- */
 wxString EscapeString( const wxString& aSource )
 {
 #if 1
@@ -316,64 +310,80 @@ wxString DateAndTime()
 }
 
 
-int StrNumCmp( const wxString& aString1, const wxString& aString2, int aLength, bool aIgnoreCase )
+int StrNumCmp( const wxString& aString1, const wxString& aString2, bool aIgnoreCase )
 {
-    int i;
     int nb1 = 0, nb2 = 0;
 
     wxString::const_iterator str1 = aString1.begin(), str2 = aString2.begin();
 
-    for( i = 0; i < aLength; i++ )
+    while( str1 != aString1.end() && str2 != aString2.end() )
     {
         wxUniChar c1 = *str1;
         wxUniChar c2 = *str2;
 
-        if( isdigit( c1 ) && isdigit( c2 ) ) /* digit found */
+        if( isdigit( c1 ) && isdigit( c2 ) ) // Both characters are digits, do numeric compare.
         {
             nb1 = 0;
             nb2 = 0;
 
-            while( isdigit( c1 ) )
+            do
             {
+                c1 = *(str1);
                 nb1 = nb1 * 10 + (int) c1 - '0';
-                c1 = *(++str1);
+                ++str1;
-            }
+            } while( str1 != aString1.end() && isdigit( *(str1) ) );
 
-            while( isdigit( c2 ) )
+            do
             {
+                c2 = *(str2);
                 nb2 = nb2 * 10 + (int) c2 - '0';
-                c2 = *(++str2);
+                ++str2;
-            }
+            } while( str2 != aString2.end() && isdigit( *(str2) ) );
 
             if( nb1 < nb2 )
                 return -1;
-            else if( nb1 > nb2 )
+
+            if( nb1 > nb2 )
                 return 1;
+
+            c1 = ( str1 != aString1.end() ) ? *(str1) : (wxUniChar) '\0';
+            c2 = ( str2 != aString2.end() ) ? *(str2) : (wxUniChar) '\0';
         }
 
+        // Any numerical comparisons to here are identical.
         if( aIgnoreCase )
         {
             if( toupper( c1 ) < toupper( c2 ) )
                 return -1;
-            else if( toupper( c1 ) > toupper( c2 ) )
+
+            if( toupper( c1 ) > toupper( c2 ) )
                 return 1;
-            else if( ( c1 == 0 ) && ( c2 == 0 ) )
-                return 0;
         }
         else
         {
             if( c1 < c2 )
                 return -1;
-            else if( c1 > c2 )
+
+            if( c1 > c2 )
                 return 1;
-            else if( ( c1 == 0 ) && ( c2 == 0 ) )
-                return 0;
         }
 
+        if( str1 != aString1.end() )
             ++str1;
+
+        if( str2 != aString2.end() )
             ++str2;
     }
 
+    if( str1 == aString1.end() && str2 != aString2.end() )
+    {
+        return -1;   // Identical to here but aString1 is longer.
+    }
+    else if( str1 != aString1.end() && str2 == aString2.end() )
+    {
+        return 1;    // Identical to here but aString2 is longer.
+    }
+
     return 0;
 }
 
@@ -486,15 +496,6 @@ bool ApplyModifier( double& value, const wxString& aString )
 }
 
 
-// Should handle:
-// a) Purely numerical e.g. '22'
-// b) Numerical with included units e.g. '15uF'
-// c) Numerical with included prefix but no units e.g. '20n'
-// d) Numerical with prefix inside number e.g. '4K7'
-// e) Other, e.g. 'MAX232'
-//
-// TODO: case (d) unimplemented !!!
-//
 int ValueStringCompare( wxString strFWord, wxString strSWord )
 {
     // Compare unescaped text
@@ -611,6 +612,7 @@ int GetTrailingInt( const wxString& aStr )
 
     // Trim and extract the trailing numeric part
     int index = aStr.Len() - 1;
+
     while( index >= 0 )
     {
         const char chr = aStr.GetChar( index );

include/footprint_info.h

@@ -168,12 +168,12 @@ protected:
 /// FOOTPRINT object list sort function.
 inline bool operator<( const FOOTPRINT_INFO& item1, const FOOTPRINT_INFO& item2 )
 {
-    int retv = StrNumCmp( item1.m_nickname, item2.m_nickname, INT_MAX, true );
+    int retv = StrNumCmp( item1.m_nickname, item2.m_nickname, true );
 
     if( retv != 0 )
         return retv < 0;
 
-    return StrNumCmp( item1.m_fpname, item2.m_fpname, INT_MAX, true ) < 0;
+    return StrNumCmp( item1.m_fpname, item2.m_fpname, true ) < 0;
 }
 
 

include/kicad_string.h

@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2004 KiCad Developers, see change_log.txt for contributors.
+ * Copyright (C) 2004, 2019 KiCad Developers, see change_log.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -48,8 +48,8 @@ wxString EscapeString( const wxString& aSource );
 wxString UnescapeString( const wxString& aSource );
 
 /**
- * Function ReadDelimitedText
+ * Copy bytes from @a aSource delimited string segment to @a aDest buffer.
- * copies bytes from @a aSource delimited string segment to @a aDest buffer.
+ *
  * The extracted string will be null terminated even if truncation is necessary
  * because aDestSize was not large enough.
  *
@@ -63,8 +63,7 @@ wxString UnescapeString( const wxString& aSource );
 int ReadDelimitedText( char* aDest, const char* aSource, int aDestSize );
 
 /**
- * Function ReadDelimitedText
+ * Copy bytes from @a aSource delimited string segment to @a aDest wxString.
- * copies bytes from @a aSource delimited string segment to @a aDest wxString.
  *
  * @param aDest is the destination wxString
  * @param aSource is the source C string holding utf8 encoded bytes.
@@ -90,48 +89,43 @@ std::string EscapedUTF8( const wxString& aString );
 wxString EscapedHTML( const wxString& aString );
 
 /**
- * Function GetLine
+ * Read one line line from \a aFile.
- * reads one line line from \a aFile.
+ *
- * @return A pointer the first useful line read by eliminating blank lines and comments.
+ * @return a pointer the first useful line read by eliminating blank lines and comments.
  */
 char* GetLine( FILE* aFile, char* Line, int* LineNum = NULL, int SizeLine = 255 );
 
 /**
- * Function StrPurge
+ * Remove leading and training spaces, tabs and end of line chars in \a text
- * removes leading and training spaces, tabs and end of line chars in \a text
+ *
- * return a pointer on the first n char in text
+ * @return a pointer on the first n char in text
  */
 char* StrPurge( char* text );
 
 /**
- * Function DateAndTime
  * @return a string giving the current date and time.
  */
 wxString DateAndTime();
 
 /**
- * Function StrNumCmp
+ * Compare two strings with alphanumerical content.
- * is a routine compatible with qsort() to sort by alphabetical order.
  *
  * This function is equivalent to strncmp() or strncasecmp() if \a aIgnoreCase is true
  * except that strings containing numbers are compared by their integer value not
- * by their ASCII code.
+ * by their ASCII code.  In other words U10 would be greater than U2.
  *
  * @param aString1 A wxString reference to the reference string.
  * @param aString2 A wxString reference to the comparison string.
- * @param aLength The number of characters to compare.  Set to -1 to compare
- *                the entire string.
  * @param aIgnoreCase Use true to make the comparison case insensitive.
  * @return An integer value of -1 if \a aString1 is less than \a aString2, 0 if
  *         \a aString1 is equal to \a aString2, or 1 if \a aString1 is greater
  *         than \a aString2.
  */
-int StrNumCmp( const wxString& aString1, const wxString& aString2, int aLength = INT_MAX,
+int StrNumCmp( const wxString& aString1, const wxString& aString2, bool aIgnoreCase = false );
-               bool aIgnoreCase = false );
 
 /**
- * Function WildCompareString
+ * Compare a string against wild card (* and ?) pattern using the usual rules.
- * compares a string against wild card (* and ?) pattern using the usual rules.
+ *
  * @return true if pattern matched otherwise false.
  */
 bool WildCompareString( const wxString& pattern,
@@ -139,21 +133,18 @@ bool WildCompareString( const wxString& pattern,
                         bool            case_sensitive = true );
 
 /**
- * Function ValueStringCompare
+ * Compare strings like the strcmp function but handle numbers and modifiers within the
- * acts just like the strcmp function but handles numbers and modifiers within the
  * string text correctly for sorting.  eg. 1mF > 55uF
- * return -1 if first string is less than the second
+ *
- * return 0 if the strings are equal
+ * @return -1 if first string is less than the second, 0 if the strings are equal, or
- * return 1 if the first string is greater than the second
+ *          1 if the first string is greater than the second.
  */
 int ValueStringCompare( wxString strFWord, wxString strSWord );
 
 /**
- * Function SplitString
+ * Breaks a string into three parts: he alphabetic preamble, the numeric part, and any
- * breaks a string into three parts.
+ * alphabetic ending.
- * The alphabetic preamble
+ *
- * The numeric part
- * Any alphabetic ending
  * For example C10A is split to C 10 A
  */
 int SplitString( wxString  strToSplit,
@@ -163,26 +154,25 @@ int SplitString( wxString  strToSplit,
 
 /**
  * Gets the trailing int, if any, from a string.
+ *
  * @param  aStr the string to check
  * @return      the trailing int or 0 if none found
  */
 int GetTrailingInt( const wxString& aStr );
 
 /**
- * Function GetIllegalFileNameWxChars
+ * @return a wxString object containing the illegal file name characters for all platforms.
- * @return a wString object containing the illegal file name characters for all platforms.
  */
 wxString GetIllegalFileNameWxChars();
 
 /**
- * Function ReplaceIllegalFileNameChars
+ * Checks \a aName for illegal file name characters.
- * checks \a aName for illegal file name characters.
  *
  * The Windows (DOS) file system forbidden characters already include the forbidden file
  * name characters for both Posix and OSX systems.  The characters \/?*|"\<\> are illegal
  * and are replaced with %xx where xx the hexadecimal equivalent of the replaced character.
  * This replacement may not be as elegant as using an underscore ('_') or hyphen ('-') but
- * it guarentees that there will be no naming conflicts when fixing footprint library names.
+ * it guarantees that there will be no naming conflicts when fixing footprint library names.
  * however, if aReplaceChar is given, it will replace the illegal chars
  *
  * @param aName is a point to a std::string object containing the footprint name to verify.
@@ -199,7 +189,7 @@ extern "C" char* strtok_r( char* str, const char* delim, char** nextp );
 
 /**
  * A helper for sorting strings from the rear.  Useful for things like 3d model names
- * where they tend to be largely repititous at the front.
+ * where they tend to be largely repetitious at the front.
  */
 struct rsort_wxString
 {

pcbnew/exporters/gen_footprints_placefile.cpp

@@ -388,7 +388,7 @@ static wxPoint File_Place_Offset;  // Offset coordinates for generated file.
 static bool sortFPlist( const LIST_MOD& ref, const LIST_MOD& tst )
 {
     if( ref.m_Layer == tst.m_Layer )
-        return StrNumCmp( ref.m_Reference, tst.m_Reference, 16 ) < 0;
+        return StrNumCmp( ref.m_Reference, tst.m_Reference ) < 0;
 
     return ref.m_Layer > tst.m_Layer;
 }

pcbnew/fp_tree_model_adapter.cpp

@@ -68,7 +68,7 @@ std::vector<LIB_TREE_ITEM*> FP_TREE_MODEL_ADAPTER::getFootprints( const wxString
     auto libBounds = std::equal_range( fullListStart, fullListEnd, dummy,
         []( const std::unique_ptr<FOOTPRINT_INFO>& a, const std::unique_ptr<FOOTPRINT_INFO>& b )
         {
-            return StrNumCmp( a->GetLibNickname(), b->GetLibNickname(), INT_MAX, true ) < 0;
+            return StrNumCmp( a->GetLibNickname(), b->GetLibNickname(), true ) < 0;
         } );
 
     for( auto i = libBounds.first; i != libBounds.second; ++i )

pcbnew/fp_tree_synchronizing_adapter.cpp

@@ -109,7 +109,7 @@ void FP_TREE_SYNCHRONIZING_ADAPTER::updateLibrary( LIB_TREE_NODE_LIB& aLibNode )
         auto footprintIt = std::lower_bound( footprints.begin(), footprints.end(), &dummy,
             []( LIB_TREE_ITEM* a, LIB_TREE_ITEM* b )
             {
-                return StrNumCmp( a->GetName(), b->GetName(), INT_MAX, true ) < 0;
+                return StrNumCmp( a->GetName(), b->GetName(), true ) < 0;
             } );
 
         if( footprintIt != footprints.end() && dummy.GetName() == (*footprintIt)->GetName() )

qa/common/test_kicad_string.cpp

@@ -102,7 +102,7 @@ BOOST_AUTO_TEST_CASE( NaturalNumberCompare )
                 c.first.first + " AND " + c.first.second + " failed for case sensitive" );
 
         BOOST_CHECK_MESSAGE(
-                StrNumCmp( c.first.first, c.first.second, INT_MAX, true ) == c.second.second,
+                StrNumCmp( c.first.first, c.first.second, true ) == c.second.second,
                 c.first.first + " AND " + c.first.second + " failed for case insensitive" );
     }
 }