Make ParseURL API safer

Summary:
The current API lets callers construct a ParseURL, and hopes they check valid.  Remove that constructor and replace with static methods:

`Expected<ParseURL, folly::Unit> parseURL(...)`

Forces the caller to check hasValue/hasError, or risk 'BadExpectedAccess exception'

`ParseURL parseURLMaybeInvalid(...)`

Gives the caller an explicit hint that they should check valid unless they know what they are doing.

Reviewed By: mjoras

Differential Revision: D30400896

fbshipit-source-id: 58ca5962040a66d77b9c9502c180150be790218e

proxygen/lib/http/HTTPMessage.cpp

@@ -546,15 +546,15 @@ bool HTTPMessage::setQueryStringImpl(const std::string& query,
                                      bool unparse,
                                      bool strict) {
   // No need to strictly verify the URL when reparsing it
-  ParseURL u(request().url_, /*strict=*/false);
+  auto u = ParseURL::parseURL(request().url_, /*strict=*/false);
 
-  if (u.valid()) {
+  if (u) {
     // Recreate the URL by just changing the query string
-    auto res = setURLImpl(createUrl(u.scheme(),
-                                    u.authority(),
-                                    u.path(),
+    auto res = setURLImpl(createUrl(u->scheme(),
+                                    u->authority(),
+                                    u->path(),
                                     query, // new query string
-                                    u.fragment()),
+                                    u->fragment()),
                           unparse,
                           strict);
     return !strict || res.valid();
@@ -970,7 +970,7 @@ const char* HTTPMessage::getDefaultReason(uint16_t status) {
 
 ParseURL HTTPMessage::setURLImplInternal(bool unparse, bool strict) {
   auto& req = request();
-  ParseURL u(req.url_, strict);
+  auto u = ParseURL::parseURLMaybeInvalid(req.url_, strict);
   if (u.valid()) {
     VLOG(9) << "set path: " << u.path() << " query:" << u.query();
     req.path_ = u.path();

proxygen/lib/http/codec/SPDYCodec.cpp

@@ -1293,9 +1293,9 @@ unique_ptr<HTTPMessage> SPDYCodec::parseHeaders(
   }
   if (direction == TransportDirection::DOWNSTREAM) {
     if (version_ == 2 && !headers.exists(HTTP_HEADER_HOST)) {
-      ParseURL url(msg->getURL(), /*strict=*/true);
-      if (url.valid()) {
-        headers.add(HTTP_HEADER_HOST, url.hostAndPort());
+      auto url = ParseURL::parseURL(msg->getURL(), /*strict=*/true);
+      if (url) {
+        headers.add(HTTP_HEADER_HOST, url->hostAndPort());
       }
     }
 

proxygen/lib/utils/ParseURL.h

@@ -22,12 +22,63 @@ namespace proxygen {
 
 class ParseURL {
  public:
-  ParseURL() {
+  /* Parse a URL.  If parsing succeeds, return a fully formed ParseURL with
+   * valid() == true.  If parsing fails, returns nothing. If you need the
+   * partial parse results, use parseURLMaybeInvalid below.
+   */
+  static folly::Expected<ParseURL, folly::Unit> parseURL(
+      folly::StringPiece urlVal, bool strict = false) noexcept {
+    ParseURL parseUrl(urlVal, strict);
+    if (parseUrl.valid()) {
+      return parseUrl;
+    } else {
+      return folly::makeUnexpected(folly::Unit());
+    }
+  }
+
+  /* Parse a URL.  Returns a ParseURL object that may or may not be valid.
+   * Caller should check valid()
+   */
+  static ParseURL parseURLMaybeInvalid(folly::StringPiece urlVal,
+                                       bool strict = false) noexcept {
+    return ParseURL(urlVal, strict);
   }
-  explicit ParseURL(folly::StringPiece urlVal, bool strict = false) noexcept {
+
+  // Deprecated.  Will be removed soon
+  explicit ParseURL(folly::StringPiece urlVal, bool strict = true) noexcept {
     init(urlVal, strict);
   }
 
+  ParseURL(ParseURL&& goner)
+      : url_(goner.url_),
+        scheme_(goner.scheme_),
+        path_(goner.path_),
+        query_(goner.query_),
+        fragment_(goner.fragment_),
+        port_(goner.port_),
+        valid_(goner.valid_),
+        initialized_(goner.initialized_) {
+    moveHostAndAuthority(std::move(goner));
+  }
+
+  ParseURL& operator=(ParseURL&& goner) {
+    url_ = goner.url_;
+    scheme_ = goner.scheme_;
+    path_ = goner.path_;
+    query_ = goner.query_;
+    fragment_ = goner.fragment_;
+    port_ = goner.port_;
+    valid_ = goner.valid_;
+    initialized_ = goner.initialized_;
+    moveHostAndAuthority(std::move(goner));
+    return *this;
+  }
+
+  ParseURL& operator=(const ParseURL&) = delete;
+  ParseURL(const ParseURL&) = delete;
+
+  ParseURL() = default;
+
   void init(folly::StringPiece urlVal, bool strict = false) {
     CHECK(!initialized_);
     url_ = urlVal;
@@ -97,6 +148,40 @@ class ParseURL {
   FB_EXPORT void stripBrackets() noexcept;
 
  private:
+  void moveHostAndAuthority(ParseURL&& goner) {
+    if (!valid_) {
+      return;
+    }
+    int64_t hostOff = -1;
+    int64_t hostNoBracketsOff = -1;
+    if (goner.host_.empty() || (goner.host_.data() >= goner.url_.data() &&
+                                goner.host_.data() < goner.url_.end())) {
+      // relative url_
+      host_ = goner.host_;
+    } else {
+      // relative authority_
+      hostOff = goner.host_.data() - goner.authority_.data();
+    }
+    if (goner.hostNoBrackets_.empty() ||
+        (goner.hostNoBrackets_.data() >= goner.url_.data() &&
+         goner.hostNoBrackets_.data() < goner.url_.end())) {
+      // relative url_
+      hostNoBrackets_ = goner.hostNoBrackets_;
+    } else {
+      // relative authority_
+      hostNoBracketsOff =
+          goner.hostNoBrackets_.data() - goner.authority_.data();
+    }
+    authority_ = std::move(goner.authority_);
+    if (hostOff >= 0) {
+      host_.reset(authority_.data() + hostOff, goner.host_.size());
+    }
+    if (hostNoBracketsOff >= 0) {
+      hostNoBrackets_.reset(authority_.data() + hostNoBracketsOff,
+                            goner.hostNoBrackets_.size());
+    }
+  }
+
   FB_EXPORT void parse(bool strict) noexcept;
 
   void parseNonFully(bool strict) noexcept;

proxygen/lib/utils/URL.h

@@ -28,7 +28,8 @@ class URL {
                Mode strict = Mode::STRICT_COMPAT) noexcept {
     valid_ = false;
 
-    ParseURL parseUrl(url, strict == Mode::STRICT);
+    ParseURL parseUrl =
+        ParseURL::parseURLMaybeInvalid(url, strict == Mode::STRICT);
 
     scheme_ = parseUrl.scheme().str();
     host_ = parseUrl.hostNoBrackets().str();

proxygen/lib/utils/test/ParseURLTest.cpp

@@ -22,7 +22,7 @@ void testParseURL(const string& url,
                   const string& expectedAuthority,
                   const bool expectedValid = true,
                   const bool strict = false) {
-  ParseURL u(url, strict);
+  auto u = ParseURL::parseURLMaybeInvalid(url, strict);
 
   if (expectedValid) {
     EXPECT_EQ(url, u.url());
@@ -40,15 +40,16 @@ void testParseURL(const string& url,
 }
 
 void testHostIsIpAddress(const string& url, const bool expected) {
-  ParseURL u(url);
+  auto u = ParseURL::parseURLMaybeInvalid(url);
+  EXPECT_TRUE(!expected || u.valid());
   EXPECT_EQ(expected, u.hostIsIPAddress());
 }
 
 TEST(ParseURL, HostNoBrackets) {
-  ParseURL p("/bar");
+  auto p = ParseURL::parseURL("/bar");
 
-  EXPECT_EQ("", p.host());
-  EXPECT_EQ("", p.hostNoBrackets());
+  EXPECT_EQ("", p->host());
+  EXPECT_EQ("", p->hostNoBrackets());
 }
 
 TEST(ParseURL, FullyFormedURL) {
@@ -241,6 +242,7 @@ TEST(ParseURL, IsHostIPAddress) {
 
 TEST(ParseURL, PortOverflow) {
   std::string url("http://foo:12345");
-  ParseURL u(folly::StringPiece(url.data(), url.size() - 4), true);
-  EXPECT_EQ(u.port(), 1);
+  auto u =
+      ParseURL::parseURL(folly::StringPiece(url.data(), url.size() - 4), true);
+  EXPECT_EQ(u->port(), 1);
 }