Use overflow-safe string-to-int parsing methods for FTP ports.

net/ftp/ftp_network_transaction.cc

Index: net/ftp/ftp_network_transaction.cc
diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index b34511fa1527419f37ca87b059dd02b2fa7040b1..5118b3c251f5d6f4b0b739951a30ed67dadc855c 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -16,6 +16,7 @@
 #include "net/base/address_list.h"
 #include "net/base/escape.h"
 #include "net/base/net_errors.h"
+#include "net/base/parse_number.h"
 #include "net/base/port_util.h"
 #include "net/base/url_util.h"
 #include "net/ftp/ftp_request_info.h"
@@ -123,25 +124,31 @@ int GetNetErrorCodeForFtpResponseCode(int response_code) {
 bool ExtractPortFromEPSVResponse(const FtpCtrlResponse& response, int* port) {
   if (response.lines.size() != 1)
     return false;
-  const char* ptr = response.lines[0].c_str();
-  while (*ptr && *ptr != '(')
-    ++ptr;
-  if (!*ptr)
-    return false;
-  char sep = *(++ptr);
-  if (!sep || isdigit(sep) || *(++ptr) != sep || *(++ptr) != sep)
+
+  base::StringPiece epsv_line(response.lines[0]);
+  size_t start = epsv_line.find('(');
+  // If the line doesn't have a '(' or doesn't have enough characters after the
+  // first '(', fail.
+  if (start == epsv_line.npos || epsv_line.length() - start < 6)

[eroman, 2016/11/28 22:51:57]

This is more commonly written as |StringPiece::npos|, can you switch to that
style? Also, may as well change the bound to 7 since that is the minimum for
success it looks like.

[mmenke, 2016/11/28 23:15:48]

Done.

     return false;
-  if (!isdigit(*(++ptr)))
+
+  // Make sure we have "(<d><d><d>...", where <d> is not a number.
+  if (isdigit(epsv_line[start + 1]) ||
+      epsv_line[start + 1] != epsv_line[start + 2] ||
+      epsv_line[start + 1] != epsv_line[start + 3]) {

[mmenke, 2016/11/28 22:28:18]

This is still ugly, but it seems a bit better to me than using raw pointers with
null checks.

[eroman, 2016/11/28 22:51:57]

Can you extract epsv_line[start+1] to a variable?

[mmenke, 2016/11/28 23:15:48]

Done.

     return false;
-  *port = *ptr - '0';
-  while (isdigit(*(++ptr))) {
-    *port *= 10;
-    *port += *ptr - '0';
   }
-  if (*ptr != sep)
+
+  // Skip over those characters.
+  start += 4;
+
+  // Make sure there's a terminal <d>.
+  size_t end = epsv_line.find(epsv_line[start - 1], start);

[eroman, 2016/11/28 22:51:57]

Same comment. Rather than using epsv_line[start-1] please use a common variable
for "<d>".

[mmenke, 2016/11/28 23:15:48]

Done.

+  if (end == epsv_line.npos)

[eroman, 2016/11/28 22:51:57]

StringPiece::npos

[mmenke, 2016/11/28 23:15:48]

Done.

     return false;
 
-  return true;
+  return ParseInt32(epsv_line.substr(start, end - start),
+                    ParseIntFormat::NON_NEGATIVE, port);
 }
 
 // There are two way we can receive IP address and port.
@@ -191,12 +198,14 @@ bool ExtractPortFromPASVResponse(const FtpCtrlResponse& response, int* port) {
   // Ignore the IP address supplied in the response. We are always going
   // to connect back to the same server to prevent FTP PASV port scanning.
   int p0, p1;
-  if (!base::StringToInt(pieces[4], &p0))
+  if (!ParseInt32(pieces[4], ParseIntFormat::NON_NEGATIVE, &p0))

[eroman, 2016/11/28 22:51:57]

ParseUint32() ?

or should we add a ParseUint8() ?

[mmenke, 2016/11/28 23:15:48]

Went with ParseUint32 (Don't think adding a new method is worth it, unless there
are other places that need it)

     return false;
-  if (!base::StringToInt(pieces[5], &p1))
+  if (!ParseInt32(pieces[5], ParseIntFormat::NON_NEGATIVE, &p1))
+    return false;
+  if (p0 >= 256 || p1 >= 256)

[eroman, 2016/11/28 22:51:57]

optional nit: I think it would be clearer writte as "p0 > 0xFF || p1 > 0xFF"

[mmenke, 2016/11/28 23:15:48]

Done.

     return false;
-  *port = (p0 << 8) + p1;
 
+  *port = (p0 << 8) + p1;
   return true;
 }