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.

2 files changed, 72 insertions, 25 deletions

diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc
index 7beb5f621..84e79122e 100644
--- 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
index d477e953c..469de4403 100644
--- 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)
 {
    {