SpdyUtils support coalescing multi-value headers.

net/quic/spdy_utils.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/spdy_utils.h"
 
 #include <memory>
 #include <vector>
 
 #include "base/stl_util.h"
@@ skipping 102 matching lines @@
 bool SpdyUtils::CopyAndValidateHeaders(const QuicHeaderList& header_list,
                                        int64_t* content_length,
                                        SpdyHeaderBlock* headers) {
   for (const auto& p : header_list) {
     const string& name = p.first;
     if (name.empty()) {
       DVLOG(1) << "Header name must not be empty.";
       return false;
     }
 
-    if (std::any_of(name.begin(), name.end(), base::IsAsciiUpper<char>)) {
-      DLOG(ERROR) << "Malformed header: Header name " << name
-                  << " contains upper-case characters.";
-      return false;
-    }

[Ryan Hamilton, 2016/05/24 21:26:05]

Did you mean to remove this?

[dahollings, 2016/05/24 22:12:16]

No, we should let that happen upon revert of revert (in fact this broke a unit
test, hence trybot failures). Thanks for the catch!

-
     auto iter = headers->find(name);
     if (iter == headers->end()) {
       (*headers)[name] = p.second;
-    } else if (name == "cookie") {
-      // Obeys section 8.1.2.5 in RFC 7540 for cookie reconstruction.
-      headers->ReplaceOrAppendHeader(
-          name, base::StringPrintf("%s; %s", iter->second.as_string().c_str(),
-                                   p.second.c_str()));
     } else {
       // This header had multiple values, so it must be reconstructed.
-      string value = base::StringPrintf(
-          "%s%c%s", iter->second.as_string().c_str(), '\0', p.second.c_str());
-      headers->ReplaceOrAppendHeader(name, value);
+      base::StringPiece v = iter->second;
+      std::string s(v.data(), v.length());

[Ryan Hamilton, 2016/05/24 22:18:46]

nit: you can just use "string" and "StringPiece" since we're doing "using
std::string" and "using base::StringPiece at the top of this file. Sorry for
missing this. Also down on line 134.

[dahollings, 2016/05/24 22:31:23]

Done.

+      if (name == "cookie") {
+        // Obeys section 8.1.2.5 in RFC 7540 for cookie reconstruction.
+        s.append("; ");
+      } else {
+        base::StringPiece("\0", 1).AppendToString(&s);
+      }
+      s.append(p.second);
+      headers->ReplaceOrAppendHeader(name, s);

[Ryan Hamilton, 2016/05/24 21:26:05]

Are you planning to make the internal version of this file match this code? I'm
curious what the bug was which makes the test pass internally but fail in the
chromium version.  Can you clarify?

[dahollings, 2016/05/24 22:12:16]

Clarification on the bug (sorry if over-detailed):

The internal version uses StrCat, which was replaced by StringPrintf in merge to
chromium. StringPrintf takes C strings, so doesn't play well with null character
delimiting. When we convert iter->second to a char *, it terminates at the first
'\0' on read, even if that was intended as a header-value delimiter instead of a
null-termination character.

Example: 
StrCat("red\0blue", '\0', "green") -> "red\0blue\0green", yay
StringPrintf("%s%c%s", "red\0", '\0', "blue\0") -> "red\0blue\0", yay 
but StringPrintf("%s%c%s", "red\0blue\0", '\0', "green\0") -> "red\0green\0",
since the C string "red\0blue\0" terminates at the first '\0'.

[Ryan Hamilton, 2016/05/24 22:18:46]

Ah, that makes sense. Should we transition the internal version of this code to
use your implementation here for consistency?

[Biren Roy, 2016/05/24 22:27:28]

I think StrCat() is great, and the internal version should continue using it.

[Ryan Hamilton, 2016/05/24 22:43:56]

In general, we try to minimize the differences between the internal an external
versions of the QUIC code so as to simplify the merge process. Is there some
reason we should no do that here?

     }
   }
 
   if (ContainsKey(*headers, "content-length")) {
     // Check whether multiple values are consistent.
     StringPiece content_length_header = (*headers)["content-length"];
     vector<string> values =
         base::SplitString(content_length_header, base::StringPiece("\0", 1),
                           base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
     for (const string& value : values) {
@@ skipping 96 matching lines @@
   return GURL(GetUrlFromHeaderBlock(headers)).host();
 }
 
 // static
 bool SpdyUtils::UrlIsValid(const SpdyHeaderBlock& headers) {
   string url(GetUrlFromHeaderBlock(headers));
   return url != "" && GURL(url).is_valid();
 }
 
 }  // namespace net