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

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

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.

/Users/vharron/ll/svn/lldb/gtest/unittest/Utility/UriParserTest.cpp

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

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

UriParser::Parse(const char* uri,
     )
 {
     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;
+    char* end = nullptr;
     int port_tmp = strtoul(port_buf, &end, 10);
-    if (*end != 0)
+    if (*end != 0 || 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;
 }