Don't consider '\0' as whitespace when parsing HTTP headers.

net/http/http_util_unittest.cc

Index: net/http/http_util_unittest.cc
diff --git a/net/http/http_util_unittest.cc b/net/http/http_util_unittest.cc
index f2c2146215ecc645bbea998d1cd7cd8002b82d0f..1ed04857c57f570ad36db6b6d19ab5fd2054221a 100644
--- a/net/http/http_util_unittest.cc
+++ b/net/http/http_util_unittest.cc
@@ -378,6 +378,7 @@ TEST(HttpUtilTest, LocateEndOfAdditionalHeaders) {
   }
 }
 TEST(HttpUtilTest, AssembleRawHeaders) {
+  // clang-format off
   struct {
     const char* const input;  // with '|' representing '\0'
     const char* const expected_result;  // with '\0' changed to '|'
@@ -679,7 +680,18 @@ TEST(HttpUtilTest, AssembleRawHeaders) {
       "HTTP/1.0 200 OK\nFoo: 1|Foo2: 3\nBar: 2\n\n",
       "HTTP/1.0 200 OK|Foo: 1Foo2: 3|Bar: 2||"
     },
+
+    // An embedded NUL should not be interpreted as LWS (leading to line
+    // continuations)
+    {
+      "HTTP/1.0 200 OK\n"
+      "Foo: 1\n"
+      "|EmbeddedNull: 3\n"
+      "Bar: 2\n\n",
+      "HTTP/1.0 200 OK|Foo: 1|EmbeddedNull: 3|Bar: 2||"

[mmenke, 2016/12/06 19:08:24]

EmbeddedNull?  Where's the magic for that.

[eroman, 2016/12/06 19:19:34]

There is nothing special about "EmbeddedNull", I just chose that name to draw
attention to what is going on in that line.

The special part is the vertical pipe '|'. This is used as shorthand in the test
for NUL [1]

[1]
https://cs.chromium.org/chromium/src/net/http/http_util_unittest.cc?q=http_ut...

[eroman, 2016/12/06 19:25:33]

Want me to rename this to something more generic like "Blah:" ?

[eroman, 2016/12/06 19:29:00]

Done.

+    },
   };
+  // clang-format on
   for (size_t i = 0; i < arraysize(tests); ++i) {
     std::string input = tests[i].input;
     std::replace(input.begin(), input.end(), '|', '\0');
@@ -1408,4 +1420,17 @@ TEST(HttpUtilTest, IsToken) {
   EXPECT_FALSE(HttpUtil::IsToken("\xff"));
 }
 
+TEST(HttpUtilTest, IsLWS) {
+  EXPECT_FALSE(HttpUtil::IsLWS('\v'));
+  EXPECT_FALSE(HttpUtil::IsLWS('\0'));
+  EXPECT_FALSE(HttpUtil::IsLWS('1'));
+  EXPECT_FALSE(HttpUtil::IsLWS('a'));
+  EXPECT_FALSE(HttpUtil::IsLWS('.'));
+  EXPECT_FALSE(HttpUtil::IsLWS('\n'));
+  EXPECT_FALSE(HttpUtil::IsLWS('\r'));
+
+  EXPECT_TRUE(HttpUtil::IsLWS('\t'));
+  EXPECT_TRUE(HttpUtil::IsLWS(' '));

[mmenke, 2016/12/06 19:08:24]

Should we have a whitelist here?  i.e.
for (int c = 0; c < 0x100; c++) {
  EXPECT_EQ(c == '\t' || c =='\t', IsLWS(static_cast<char>(c)));
}

(Or go from -0x80 to < 0x80, if you want to avoid the overflow)

[eroman, 2016/12/06 19:19:34]

I think that is a bit overkill, but can switch to explicit enumeration if you
would like.

My thinking given the implementation (and bug fix) was to spot check a couple
character classes

[mmenke, 2016/12/06 19:25:29]

ok

+}
+
 }  // namespace net