Fix a bug that the last CRLF of chunked body may be used in the next request (#1812)

* Fix a bug that the last CRLF of chunked body may be used in the next request

The last CRLF of chunked body is checked by #1607. But it's
incomplete. If a client sends the last CRLF (or just LF) after Puma
processes "0\r\n" line, the last CRLF (or just LF) isn't dropped in
the "0\r\n" process:

675344e860/lib/puma/client.rb (L183-L192)

    if line.end_with?("\r\n")
      len = line.strip.to_i(16)
      if len == 0
        @body.rewind
        rest = io.read
        # rest is "" with no the last CRLF case and
        # "\r" with no last LF case.
        # rest.start_with?("\r\n") returns false for
        # Both of these cases.
        rest = rest[2..-1] if rest.start_with?("\r\n")
        @buffer = rest.empty? ? nil : rest
        set_ready
        return true
      end

The unprocessed last CRLF (or LF) is used as the first data in the
next request. Because Puma::Client#reset sets `@parsed_bytes`
to 0.

675344e860/lib/puma/client.rb (L100-L109)

    def reset(fast_check=true)
      @parsed_bytes = 0

It means that data in `@buffer` (it's "\r" in no the last LF case) and
unread data in input socket (it's "\r\n" in no the last CRLF case and
"\n" in no the last LF case) are used used as the first data in the
next request.

This change fixes these cases by the followings:

  * Ensures reading the last CRLF by setting `@partial_part_left` when
    CRLF isn't read in processing "0\r\n" line.

  * Introduces a `@in_last_chunk` new state to detect whether the last
    CRLF is waiting or not. It's reset in Puma::Client#reset.

* Remove unnecessary returns

https://github.com/puma/puma/pull/1812#discussion_r307806310 is the
location where this rule is made.

* Add missing last CRLF for chunked request in tests

lib/puma/client.rb

@@ -68,6 +68,8 @@ module Puma
       @remote_addr_header = nil
 
       @body_remain = 0
+
+      @in_last_chunk = false
     end
 
     attr_reader :env, :to_io, :body, :io, :timeout_at, :ready, :hijacked,
@@ -108,6 +110,7 @@ module Puma
       @ready = false
       @body_remain = 0
       @peerip = nil
+      @in_last_chunk = false
 
       if @buffer
         @parsed_bytes = @parser.execute(@env, @buffer, @parsed_bytes)
@@ -166,7 +169,7 @@ module Puma
           chunk = chunk[@partial_part_left..-1]
           @partial_part_left = 0
         else
-          @body << chunk
+          @body << chunk if @partial_part_left > 2 # don't include the last \r\n
           @partial_part_left -= chunk.size
           return false
         end
@@ -184,12 +187,20 @@ module Puma
         if line.end_with?("\r\n")
           len = line.strip.to_i(16)
           if len == 0
+            @in_last_chunk = true
             @body.rewind
             rest = io.read
-            rest = rest[2..-1] if rest.start_with?("\r\n")
-            @buffer = rest.empty? ? nil : rest
-            set_ready
-            return true
+            last_crlf_size = "\r\n".bytesize
+            if rest.bytesize < last_crlf_size
+              @buffer = nil
+              @partial_part_left = last_crlf_size - rest.bytesize
+              return false
+            else
+              @buffer = rest[last_crlf_size..-1]
+              @buffer = nil if @buffer.empty?
+              set_ready
+              return true
+            end
           end
 
           len += 2
@@ -219,7 +230,12 @@ module Puma
         end
       end
 
-      return false
+      if @in_last_chunk
+        set_ready
+        true
+      else
+        false
+      end
     end
 
     def read_chunked_body

test/test_puma_server.rb

@@ -590,7 +590,7 @@ EOF
     @server.run
 
     sock = TCPSocket.new @host, @server.connected_port
-    sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n"
+    sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -612,7 +612,7 @@ EOF
     sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\n"
     sleep 1
 
-    sock << "h\r\n4\r\nello\r\n0\r\n"
+    sock << "h\r\n4\r\nello\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -634,7 +634,7 @@ EOF
     sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n"
     sleep 1
 
-    sock << "4\r\nello\r\n0\r\n"
+    sock << "4\r\nello\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -656,7 +656,7 @@ EOF
     sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r"
     sleep 1
 
-    sock << "\nh\r\n4\r\nello\r\n0\r\n"
+    sock << "\nh\r\n4\r\nello\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -678,7 +678,7 @@ EOF
     sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1"
     sleep 1
 
-    sock << "\r\nh\r\n4\r\nello\r\n0\r\n"
+    sock << "\r\nh\r\n4\r\nello\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -700,7 +700,7 @@ EOF
     sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n4\r\ne"
     sleep 1
 
-    sock << "llo\r\n0\r\n"
+    sock << "llo\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -796,7 +796,7 @@ EOF
     @server.run
 
     sock = TCPSocket.new @host, @server.connected_port
-    sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: Chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n"
+    sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: Chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n\r\n"
 
     data = sock.read
 
@@ -836,11 +836,23 @@ EOF
     @server.run
 
     sock = TCPSocket.new @host, @server.connected_port
-    sock << "GET / HTTP/1.1\r\nConnection: Keep-Alive\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n\r\n"
+    sock << "GET / HTTP/1.1\r\nConnection: Keep-Alive\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n"
+
+    last_crlf_written = false
+    last_crlf_writer = Thread.new do
+      sleep 0.1
+      sock << "\r"
+      sleep 0.1
+      sock << "\n"
+      last_crlf_written = true
+    end
 
     h = header(sock)
     assert_equal ["HTTP/1.1 200 OK", "Content-Length: 0"], h
     assert_equal "hello", body
+    assert_equal true, last_crlf_written
+
+    last_crlf_writer.join
 
     sock << "GET / HTTP/1.1\r\nConnection: Keep-Alive\r\nTransfer-Encoding: chunked\r\n\r\n4\r\ngood\r\n3\r\nbye\r\n0\r\n\r\n"
     sleep 0.1
@@ -934,7 +946,7 @@ EOF
     sock = TCPSocket.new @host, @server.connected_port
     sock << "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n"
     sleep 1
-    sock << "4\r\nello\r\n0\r\n"
+    sock << "4\r\nello\r\n0\r\n\r\n"
 
     sock.gets