SpdyUtils support coalescing multi-value headers.

net/quic/spdy_utils.cc

Index: net/quic/spdy_utils.cc
diff --git a/net/quic/spdy_utils.cc b/net/quic/spdy_utils.cc
index 90e65373827dc08b0052c52e8e019117d7f82813..c73dbb6eccdaf15ebbcbf8c522fb218e63e41fb6 100644
--- a/net/quic/spdy_utils.cc
+++ b/net/quic/spdy_utils.cc
@@ -120,25 +120,21 @@ bool SpdyUtils::CopyAndValidateHeaders(const QuicHeaderList& header_list,
       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?

     }
   }