UriParser - fixed potential buffer overrun

[vharron]

Summary

fixed potential buffer overrun by adding "10" to port parameter in sscanf
return error if port is invalid (>65535)
added tests

[ovyalov]

/Users/vharron/ll/svn/lldb/source/Utility/UriParser.cpp

42 ↗

(On Diff #17987)

You may define port_tmp as auto instead of integer - otherwise if result of strtoul is greater than MAX_INT but less than max of unsigned long int it might be just a negative number.

43 ↗

(On Diff #17987)

Check for portr_tmp <= 0?

[vharron]

strtoul == "string to unsigned long" only accepts positive numbers

[clayborg]

I would prefer this to use Args::StringToUInt32() instead of a manual call to strtoul() from #include "lldb/Interpreter/Args.h" (even though the correct usage of strtoul() is used here). This helps abstract us from the host we are running on in case there is no strtoul() and also leads me to the fact that the Args::StringTo* calls could actually be moved into the host layer.

You might also be better off using the RegularExpression class to parse these instead of sscanf, but this isn't a required change.

[vharron]

Switched from ::strtoul to StringConvert::ToUInt32

[clayborg]

Looks good.

[vharron]

Changed port output parameter to be -1 if port is unspecified

This makes more sense because we might want to be able to
differentiate between port unspecified and port==0

[vharron]

Committed revision 226204.

gtest/unittest/Utility/Makefile

 THIS_FILE_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))/
 
 LEVEL := $(realpath $(THIS_FILE_DIR)../../make)
 
 CFLAGS_EXTRAS := -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_CONSTANT_MACROS
 ENABLE_THREADS := YES
 CXX_SOURCES := $(wildcard *.cpp) \
+  $(realpath $(LEVEL)/../../source/Host/common/StringConvert.cpp) \
   $(realpath $(LEVEL)/../../source/Utility/StringExtractor.cpp) \
   $(realpath $(LEVEL)/../../source/Utility/UriParser.cpp)
 MAKE_DSYM := NO
 
 OS := $(shell uname -s)
 # $(info OS $(OS))
 ifeq ($(OS),Linux)
     LD_EXTRAS := -lncurses -ldl
 endif
 
 include $(LEVEL)/Makefile.rules

gtest/unittest/Utility/UriParserTest.cpp

#define VALIDATE \
     EXPECT_EQ (testCase.m_result, UriParser::Parse(testCase.m_uri, scheme, hostname, port, path)); \
     EXPECT_STREQ (testCase.m_scheme, scheme.c_str()); \
     EXPECT_STREQ (testCase.m_hostname, hostname.c_str()); \
     EXPECT_EQ (testCase.m_port, port); \
     EXPECT_STREQ (testCase.m_path, path.c_str());
 
 TEST_F (UriParserTest, Minimal)
 {
-    const UriTestCase testCase("x://y", "x", "y", 0, "/");
+    const UriTestCase testCase("x://y", "x", "y", -1, "/");
     VALIDATE
 }
 
 TEST_F (UriParserTest, MinimalPort)
 {
     const UriTestCase testCase("x://y:1", "x", "y", 1, "/");
     VALIDATE
 }
 
 TEST_F (UriParserTest, MinimalPath)
 {
-    const UriTestCase testCase("x://y/", "x", "y", 0, "/");
+    const UriTestCase testCase("x://y/", "x", "y", -1, "/");
     VALIDATE
 }
 
 TEST_F (UriParserTest, MinimalPortPath)
 {
     const UriTestCase testCase("x://y:1/", "x", "y", 1, "/");
     VALIDATE
 }

 }
 
 TEST_F (UriParserTest, Empty)
 {
     const UriTestCase testCase("");
     VALIDATE
 }
 
+TEST_F (UriParserTest, PortOverflow)
+{
+    const UriTestCase testCase("x://y:0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/");
+    VALIDATE
+}
+

source/Utility/UriParser.h

 #include <string>
 
 // Other libraries and framework includes
 // Project includes
 
 class UriParser
 {
 public:
+    // Parses
+    // RETURN VALUE
+    //   if url is valid, function returns true and 
+    //   scheme/hostname/port/path are set to the parsed values
+    //   port it set to -1 if it is not included in the URL
+    //
+    //   if the url is invalid, function returns false and
+    //   output parameters remain unchanged
     static bool Parse(const char* uri,
         std::string& scheme,
         std::string& hostname,
         int& port,
         std::string& path
         );
 };
 
 #endif  // utility_UriParser_h_

source/Utility/UriParser.cpp

 #include "Utility/UriParser.h"
 
 // C Includes
 #include <stdlib.h>
 
 // C++ Includes
 // Other libraries and framework includes
 // Project includes
+#include "lldb/Host/StringConvert.h"
+
+using namespace lldb_private;
 
 //----------------------------------------------------------------------
 // UriParser::Parse
 //----------------------------------------------------------------------
 bool
 UriParser::Parse(const char* uri,
     std::string& scheme,
     std::string& hostname,
     int& port,
     std::string& path
     )
 {
     char scheme_buf[100] = {0};
     char hostname_buf[256] = {0};
     char port_buf[11] = {0}; // 10==strlen(2^32)
     char path_buf[2049] = {'/', 0};
   
     bool ok = false;
-         if (4==sscanf(uri, "%99[^:/]://%255[^/:]:%[^/]/%2047s", scheme_buf, hostname_buf, port_buf, path_buf+1)) { ok = true; }
-    else if (3==sscanf(uri, "%99[^:/]://%255[^/:]:%[^/]", scheme_buf, hostname_buf, port_buf)) { ok = true; }
+         if (4==sscanf(uri, "%99[^:/]://%255[^/:]:%10[^/]/%2047s", scheme_buf, hostname_buf, port_buf, path_buf+1)) { ok = true; }
+    else if (3==sscanf(uri, "%99[^:/]://%255[^/:]:%10[^/]", scheme_buf, hostname_buf, port_buf)) { ok = true; }
     else if (3==sscanf(uri, "%99[^:/]://%255[^/]/%2047s", scheme_buf, hostname_buf, path_buf+1)) { ok = true; }
     else if (2==sscanf(uri, "%99[^:/]://%255[^/]", scheme_buf, hostname_buf)) { ok = true; }
 
-    char* end = port_buf;
-    int port_tmp = strtoul(port_buf, &end, 10);
-    if (*end != 0)
+    bool success = false;
+    int port_tmp = -1;
+    if (port_buf[0])
+    {
+        port_tmp = StringConvert::ToUInt32(port_buf, UINT32_MAX, 10, &success);
+        if (!success || port_tmp > 65535)
         {
             // there are invalid characters in port_buf
             return false;
         }
+    }
 
     if (ok)
     {
         scheme.assign(scheme_buf);
         hostname.assign(hostname_buf);
         port = port_tmp;
         path.assign(path_buf);
     }
     return ok;
 }