Make multipart/x-mixed-replace parser more robust

1. The parser eats leading CR, LF, CRLF and LFLF. It would be better to
eat only LF and CRLF, as other parts such as
net::HttpUtil::LocateEndOfHeaders does the same thing.

2. The parser eats leading EOLs repeatedly. For example, it eats "\n"
when "\n\nfoo" is given, but it eats "\n" twice when "\n" is given
and then "\nfoo" is given.

3. The parser sends the remainder inputs to the client as body data
if it is parsing body and the input ends with LF, although the parser
might be ought to ignore that LF in the future if it's the end-of-body.

BUG=570608

Review URL: https://codereview.chromium.org/1808353003

Cr-Commit-Position: refs/heads/master@{#382790}

4 files changed, 100 insertions, 48 deletions

diff --git a/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp b/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
index aa4596d..12030d6 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
+++ b/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
@@ -123,11 +123,15 @@ TEST(ImageResourceTest, MultipartImage)
     // The first bytes arrive. The data buffer is created, but no image is created.
     cachedImage->appendData(secondPart, strlen(secondPart));
     ASSERT_TRUE(cachedImage->resourceBuffer());
-    ASSERT_EQ(cachedImage->resourceBuffer()->size(), strlen(secondPart));
     ASSERT_FALSE(cachedImage->hasImage());
     ASSERT_EQ(client.imageChangedCount(), 0);
     ASSERT_FALSE(client.notifyFinishedCalled());
 
+    const char thirdPart[] = "--boundary";
+    cachedImage->appendData(thirdPart, strlen(thirdPart));
+    ASSERT_TRUE(cachedImage->resourceBuffer());
+    ASSERT_EQ(cachedImage->resourceBuffer()->size(), strlen(secondPart) - 1);
+
     // This part finishes. The image is created, callbacks are sent, and the data buffer is cleared.
     cachedImage->finish();
     ASSERT_FALSE(cachedImage->resourceBuffer());
diff --git a/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp b/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp
index cfbee3c..69080f0 100644
--- a/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp
+++ b/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp
@@ -35,15 +35,15 @@ void MultipartImageResourceParser::appendData(const char* bytes, size_t size)
 
     if (m_isParsingTop) {
         // Eat leading \r\n
-        size_t pos = pushOverLine(m_data, 0);
-        if (pos)
-            m_data.remove(0, pos);
-
-        if (m_data.size() < m_boundary.size() + 2) {
+        size_t pos = skippableLength(m_data, 0);
+        // +2 for "--"
+        if (m_data.size() < m_boundary.size() + 2 + pos) {
             // We don't have enough data yet to make a boundary token.  Just
             // wait until the next chunk of data arrives.
             return;
         }
+        if (pos)
+            m_data.remove(0, pos);
 
         // Some servers don't send a boundary token before the first chunk of
         // data.  We handle this case anyway (Gecko does too).
@@ -56,11 +56,6 @@ void MultipartImageResourceParser::appendData(const char* bytes, size_t size)
 
     // Headers
     if (m_isParsingHeaders) {
-        // Eat leading \r\n
-        size_t pos = pushOverLine(m_data, 0);
-        if (pos)
-            m_data.remove(0, pos);
-
         if (!parseHeaders()) {
             // Get more data before trying again.
             return;
@@ -96,8 +91,7 @@ void MultipartImageResourceParser::appendData(const char* bytes, size_t size)
         }
 
         // We can now throw out data up through the boundary
-        size_t offset = pushOverLine(m_data, boundaryEndPosition);
-        m_data.remove(0, boundaryEndPosition + offset);
+        m_data.remove(0, boundaryEndPosition);
 
         // Ok, back to parsing headers
         if (!parseHeaders()) {
@@ -108,14 +102,11 @@ void MultipartImageResourceParser::appendData(const char* bytes, size_t size)
             return;
     }
 
-    // At this point, we should send over any data we have, but keep enough data
-    // buffered to handle a boundary that may have been truncated.
-    if (!m_isParsingHeaders && m_data.size() > m_boundary.size()) {
-        // If the last character is a new line character, go ahead and just send
-        // everything we have buffered.  This matches an optimization in Gecko.
-        size_t sendLength = m_data.size() - m_boundary.size();
-        if (m_data.last() == '\n')
-            sendLength = m_data.size();
+    // At this point, we should send over any data we have, but keep enough
+    // data buffered to handle a boundary that may have been truncated.
+    // "+2" for CRLF, as we may ignore the last CRLF.
+    if (!m_isParsingHeaders && m_data.size() > m_boundary.size() + 2) {
+        size_t sendLength = m_data.size() - m_boundary.size() - 2;
         m_client->multipartDataReceived(m_data.data(), sendLength);
         m_data.remove(0, sendLength);
     }
@@ -134,23 +125,20 @@ void MultipartImageResourceParser::finish()
     m_sawLastBoundary = true;
 }
 
-size_t MultipartImageResourceParser::pushOverLine(const Vector<char>& data, size_t pos)
+size_t MultipartImageResourceParser::skippableLength(const Vector<char>& data, size_t pos)
 {
-    size_t offset = 0;
-    // TODO(yhirano): This function has two problems. Fix them.
-    //  1. It eats "\n\n".
-    //  2. When the incoming data is not sufficient (i.e. data[pos] == '\r'
-    //     && data.size() == pos + 1), it should notify the caller.
-    if (pos < data.size() && (data[pos] == '\r' || data[pos] == '\n')) {
-        ++offset;
-        if (pos + 1 < data.size() && data[pos + 1] == '\n')
-            ++offset;
-    }
-    return offset;
+    if (data.size() >= pos + 2 && data[pos] == '\r' && data[pos + 1] == '\n')
+        return 2;
+    if (data.size() >= pos + 1 && data[pos] == '\n')
+        return 1;
+    return 0;
 }
 
 bool MultipartImageResourceParser::parseHeaders()
 {
+    // Eat leading \r\n
+    size_t pos = skippableLength(m_data, 0);
+
     // Create a WebURLResponse based on the original set of headers + the
     // replacement headers. We only replace the same few headers that gecko
     // does. See netwerk/streamconv/converters/nsMultiMixedConv.cpp.
@@ -159,9 +147,9 @@ bool MultipartImageResourceParser::parseHeaders()
         response.addHTTPHeaderField(header.key, header.value);
 
     size_t end = 0;
-    if (!Platform::current()->parseMultipartHeadersFromBody(m_data.data(), m_data.size(), &response, &end))
+    if (!Platform::current()->parseMultipartHeadersFromBody(m_data.data() + pos, m_data.size() - pos, &response, &end))
         return false;
-    m_data.remove(0, end);
+    m_data.remove(0, end + pos);
     // Send the response!
     m_client->onePartInMultipartReceived(response.toResourceResponse());
     return true;
diff --git a/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h b/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h
index c59e7a3..6b39fde 100644
--- a/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h
+++ b/third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h
@@ -67,13 +67,13 @@ public:
 
     DECLARE_TRACE();
 
-    static size_t pushOverLineForTest(const Vector<char>& data, size_t size) { return pushOverLine(data, size); }
+    static size_t skippableLengthForTest(const Vector<char>& data, size_t size) { return skippableLength(data, size); }
     static size_t findBoundaryForTest(const Vector<char>& data, Vector<char>* boundary) { return findBoundary(data, boundary); }
 
 private:
     bool parseHeaders();
     bool isCancelled() const { return m_isCancelled; }
-    static size_t pushOverLine(const Vector<char>&, size_t);
+    static size_t skippableLength(const Vector<char>&, size_t);
     // This function updates |*boundary|.
     static size_t findBoundary(const Vector<char>& data, Vector<char>* boundary);
 
diff --git a/third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp b/third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp
index bf4a564..93c1974 100644
--- a/third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp
+++ b/third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp
@@ -43,7 +43,7 @@ public:
     Vector<Vector<char>> m_data;
 };
 
-TEST(MultipartResponseTest, PushOverLine)
+TEST(MultipartResponseTest, SkippableLength)
 {
     struct {
         const char* input;
@@ -55,19 +55,19 @@ TEST(MultipartResponseTest, PushOverLine)
         { "Line", 10, 0 },
         { "\r\nLine", 0, 2 },
         { "\nLine", 0, 1 },
-        { "\n\nLine", 0, 2 },
-        { "\rLine", 0, 1 },
+        { "\n\nLine", 0, 1 },
+        { "\rLine", 0, 0 },
         { "Line\r\nLine", 4, 2 },
         { "Line\nLine", 4, 1 },
-        { "Line\n\nLine", 4, 2 },
-        { "Line\rLine", 4, 1 },
-        { "Line\r\rLine", 4, 1 },
+        { "Line\n\nLine", 4, 1 },
+        { "Line\rLine", 4, 0 },
+        { "Line\r\rLine", 4, 0 },
     };
     for (size_t i = 0; i < WTF_ARRAY_LENGTH(lineTests); ++i) {
         Vector<char> input;
         input.append(lineTests[i].input, strlen(lineTests[i].input));
         EXPECT_EQ(lineTests[i].expected,
-            MultipartImageResourceParser::pushOverLineForTest(input, lineTests[i].position));
+            MultipartImageResourceParser::skippableLengthForTest(input, lineTests[i].position));
     }
 }
 
@@ -201,7 +201,7 @@ TEST(MultipartResponseTest, NoEndBoundary)
     parser->appendData(data, strlen(data));
     ASSERT_EQ(1u, client->m_responses.size());
     ASSERT_EQ(1u, client->m_data.size());
-    EXPECT_EQ("This is a sample response\n", toString(client->m_data[0]));
+    EXPECT_EQ("This is a sample ", toString(client->m_data[0]));
 
     parser->finish();
     ASSERT_EQ(1u, client->m_responses.size());
@@ -226,7 +226,7 @@ TEST(MultipartResponseTest, NoStartAndEndBoundary)
     parser->appendData(data, strlen(data));
     ASSERT_EQ(1u, client->m_responses.size());
     ASSERT_EQ(1u, client->m_data.size());
-    EXPECT_EQ("This is a sample response\n", toString(client->m_data[0]));
+    EXPECT_EQ("This is a sample ", toString(client->m_data[0]));
 
     parser->finish();
     ASSERT_EQ(1u, client->m_responses.size());
@@ -322,7 +322,7 @@ TEST(MultipartResponseTest, BreakInBoundary)
     // Break in first and second
     const TestChunk bound2[] = {
         { 0, 4, 0, "" },
-        { 4, 55, 1, "datadatadatadat" },
+        { 4, 55, 1, "datadatadatad" },
         { 55, 65, 1, "datadatadatadatadata" },
         { 65, 110, 2, "foofoofoofoofoo" },
     };
@@ -330,7 +330,7 @@ TEST(MultipartResponseTest, BreakInBoundary)
 
     // Break in second only
     const TestChunk bound3[] = {
-        { 0, 55, 1, "datadatadatadat" },
+        { 0, 55, 1, "datadatadatad" },
         { 55, 110, 2, "foofoofoofoofoo" },
     };
     variousChunkSizesTest(bound3, 2, 3, "foofoofoofoofoo");
@@ -442,6 +442,66 @@ TEST(MultipartResponseTest, MultipleBoundaries)
     EXPECT_EQ("foofoo", toString(client->m_data[1]));
 }
 
+TEST(MultipartResponseTest, EatLeadingLF)
+{
+    ResourceResponse response;
+    response.setMimeType("multipart/x-mixed-replace");
+    MockClient* client = new MockClient;
+    Vector<char> boundary;
+    boundary.append("bound", 5);
+
+    const char data[] =
+        "\n\n\n--bound\n\n\ncontent-type: 1\n\n"
+        "\n\n\n--bound\n\ncontent-type: 2\n\n"
+        "\n\n\n--bound\ncontent-type: 3\n\n";
+    MultipartImageResourceParser* parser = new MultipartImageResourceParser(response, boundary, client);
+
+    for (size_t i = 0; i < strlen(data); ++i)
+        parser->appendData(&data[i], 1);
+    parser->finish();
+
+    ASSERT_EQ(4u, client->m_responses.size());
+    ASSERT_EQ(4u, client->m_data.size());
+    EXPECT_EQ(String(), client->m_responses[0].httpHeaderField("content-type"));
+    EXPECT_EQ("", toString(client->m_data[0]));
+    EXPECT_EQ(String(), client->m_responses[1].httpHeaderField("content-type"));
+    EXPECT_EQ("\ncontent-type: 1\n\n\n\n", toString(client->m_data[1]));
+    EXPECT_EQ(String(), client->m_responses[2].httpHeaderField("content-type"));
+    EXPECT_EQ("content-type: 2\n\n\n\n", toString(client->m_data[2]));
+    EXPECT_EQ("3", client->m_responses[3].httpHeaderField("content-type"));
+    EXPECT_EQ("", toString(client->m_data[3]));
+}
+
+TEST(MultipartResponseTest, EatLeadingCRLF)
+{
+    ResourceResponse response;
+    response.setMimeType("multipart/x-mixed-replace");
+    MockClient* client = new MockClient;
+    Vector<char> boundary;
+    boundary.append("bound", 5);
+
+    const char data[] =
+        "\r\n\r\n\r\n--bound\r\n\r\n\r\ncontent-type: 1\r\n\r\n"
+        "\r\n\r\n\r\n--bound\r\n\r\ncontent-type: 2\r\n\r\n"
+        "\r\n\r\n\r\n--bound\r\ncontent-type: 3\r\n\r\n";
+    MultipartImageResourceParser* parser = new MultipartImageResourceParser(response, boundary, client);
+
+    for (size_t i = 0; i < strlen(data); ++i)
+        parser->appendData(&data[i], 1);
+    parser->finish();
+
+    ASSERT_EQ(4u, client->m_responses.size());
+    ASSERT_EQ(4u, client->m_data.size());
+    EXPECT_EQ(String(), client->m_responses[0].httpHeaderField("content-type"));
+    EXPECT_EQ("", toString(client->m_data[0]));
+    EXPECT_EQ(String(), client->m_responses[1].httpHeaderField("content-type"));
+    EXPECT_EQ("\r\ncontent-type: 1\r\n\r\n\r\n\r\n", toString(client->m_data[1]));
+    EXPECT_EQ(String(), client->m_responses[2].httpHeaderField("content-type"));
+    EXPECT_EQ("content-type: 2\r\n\r\n\r\n\r\n", toString(client->m_data[2]));
+    EXPECT_EQ("3", client->m_responses[3].httpHeaderField("content-type"));
+    EXPECT_EQ("", toString(client->m_data[3]));
+}
+
 } // namespace
 
 } // namespace blink