Forbid negative values in unsigned StrToNum explicitly

strtoul(l) surprises us with parsing negative values which should not exist in the places we use to parse them, so we can just downright refuse them rather than trying to work with them by having them promoted to huge positive values.
Team5964 · Feb 3, 2021 · e0743a8 · e0743a8
1 parent 6630c3e
commit e0743a8
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 25 deletions.
diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc
@@ -23,6 +23,7 @@
 #include <algorithm>
 #include <array>
 #include <iomanip>
+#include <limits>
 #include <locale>
 #include <sstream>
 #include <sstream>
@@ -1139,34 +1140,24 @@ bool FTPMDTMStrToTime(const char* const str,time_t &time)
 									/*}}}*/
 // StrToNum - Convert a fixed length string to a number			/*{{{*/
 // ---------------------------------------------------------------------
-/* This is used in decoding the crazy fixed length string headers in 
+/* This is used in decoding the crazy fixed length string headers in
    tar and ar files. */
 bool StrToNum(const char *Str,unsigned long &Res,unsigned Len,unsigned Base)
 {
-   char S[30];
-   if (Len >= sizeof(S))
+   unsigned long long BigRes;
+   if (not StrToNum(Str, BigRes, Len, Base))
       return false;
-   memcpy(S,Str,Len);
-   S[Len] = 0;
-
-   // All spaces is a zero
-   Res = 0;
-   unsigned I;
-   for (I = 0; S[I] == ' '; I++);
-   if (S[I] == 0)
-      return true;
-
-   char *End;
-   Res = strtoul(S,&End,Base);
-   if (End == S)
+
+   if (std::numeric_limits<unsigned long>::max() < BigRes)
       return false;
-
+
+   Res = BigRes;
    return true;
 }
 									/*}}}*/
 // StrToNum - Convert a fixed length string to a number			/*{{{*/
 // ---------------------------------------------------------------------
-/* This is used in decoding the crazy fixed length string headers in 
+/* This is used in decoding the crazy fixed length string headers in
    tar and ar files. */
 bool StrToNum(const char *Str,unsigned long long &Res,unsigned Len,unsigned Base)
 {
@@ -1175,20 +1166,20 @@ bool StrToNum(const char *Str,unsigned long long &Res,unsigned Len,unsigned Base
       return false;
    memcpy(S,Str,Len);
    S[Len] = 0;
-   
+
    // All spaces is a zero
    Res = 0;
    unsigned I;
-   for (I = 0; S[I] == ' '; I++);
+   for (I = 0; S[I] == ' '; ++I);
    if (S[I] == 0)
       return true;
-
+   if (S[I] == '-')
+      return false;
+
    char *End;
+   errno = 0;
    Res = strtoull(S,&End,Base);
-   if (End == S)
-      return false;
-
-   return true;
+   return not (End == S || errno != 0);
 }
 									/*}}}*/
 

diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc
@@ -1,6 +1,8 @@
 #include <config.h>
 #include <apt-pkg/fileutl.h>
+#include <apt-pkg/string_view.h>
 #include <apt-pkg/strutl.h>
+#include <limits>
 #include <string>
 #include <vector>
 
@@ -257,6 +259,60 @@ TEST(StrUtilTest,QuoteString)
    EXPECT_EQ("Eltville-Erbach", DeQuoteString(QuoteString("Eltville-Erbach", "")));
 }
 
+static void EXPECT_STRTONUM(APT::StringView const str, bool const success, unsigned long const expected, unsigned const base)
+{
+   SCOPED_TRACE(std::string(str.data(), str.length()));
+   SCOPED_TRACE(base);
+   unsigned long N1 = 1000;
+   unsigned long long N2 = 1000;
+   if (not success)
+   {
+      EXPECT_FALSE(StrToNum(str.data(), N1, str.length(), base));
+      EXPECT_FALSE(StrToNum(str.data(), N2, str.length(), base));
+      return;
+   }
+   EXPECT_TRUE(StrToNum(str.data(), N1, str.length(), base));
+   EXPECT_EQ(expected, N1);
+
+   EXPECT_TRUE(StrToNum(str.data(), N2, str.length(), base));
+   EXPECT_EQ(expected, N2);
+}
+TEST(StrUtilTest,StrToNum)
+{
+   EXPECT_STRTONUM("", true, 0, 10);
+   EXPECT_STRTONUM("  ", true, 0, 10);
+   EXPECT_STRTONUM("0", true, 0, 10);
+   EXPECT_STRTONUM("1", true, 1, 10);
+   EXPECT_STRTONUM(" 1 ", true, 1, 10);
+   EXPECT_STRTONUM("1", true, 1, 8);
+   EXPECT_STRTONUM("10", true, 10, 10);
+   EXPECT_STRTONUM("10", true, 8, 8);
+   EXPECT_STRTONUM("010", true, 8, 8);
+   EXPECT_STRTONUM(" 010 ", true, 8, 8);
+   EXPECT_STRTONUM("-1", false, 0, 10);
+   EXPECT_STRTONUM(" -1 ", false, 0, 10);
+   EXPECT_STRTONUM("11", true, 3, 2);
+
+   unsigned long long bigN = 0;
+   unsigned long smallN = 0;
+   auto bigLimit = std::to_string(std::numeric_limits<unsigned long long>::max());
+   if (std::numeric_limits<unsigned long>::max() < std::numeric_limits<unsigned long long>::max())
+   {
+      EXPECT_TRUE(StrToNum(bigLimit.c_str(), bigN, bigLimit.length(), 10));
+      EXPECT_EQ(std::numeric_limits<unsigned long long>::max(), bigN);
+      EXPECT_FALSE(StrToNum(bigLimit.c_str(), smallN, bigLimit.length(), 10));
+   }
+   bigLimit.append("0");
+   EXPECT_FALSE(StrToNum(bigLimit.c_str(), bigN, bigLimit.length(), 10));
+   EXPECT_FALSE(StrToNum(bigLimit.c_str(), smallN, bigLimit.length(), 10));
+
+   auto const smallLimit = std::to_string(std::numeric_limits<unsigned long>::max());
+   EXPECT_TRUE(StrToNum(smallLimit.c_str(), bigN, smallLimit.length(), 10));
+   EXPECT_EQ(std::numeric_limits<unsigned long>::max(), bigN);
+   EXPECT_TRUE(StrToNum(smallLimit.c_str(), smallN, smallLimit.length(), 10));
+   EXPECT_EQ(std::numeric_limits<unsigned long>::max(), smallN);
+}
+
 TEST(StrUtilTest,RFC1123StrToTime)
 {
    {