lib/client.rb - use Tempfile.create instead of Tempfile.new for reque…

…st bodies (#3296)

Currently, `Tempfile.new` is used in `Client` when a request contains content (a body) that is either very large or indeterminate (chunked).  `Tempfile.new` creates a finalizer that removes the `Tempfile`, so its removal is dependent on GC.

Also, in general, I think we'd prefer that Puma does its own cleanup, and not depend on GC to do so, at least with system resources like IO (files, pipes, sockets, etc).

So, use `Tempfile.create` and remove the file when Puma is finished with it.

lib/puma/client.rb

@@ -160,8 +160,6 @@ def reset(fast_check=true)
       @read_header = true
       @read_proxy = !!@expect_proxy_proto
       @env = @proto_env.dup
-      @body = nil
-      @tempfile = nil
       @parsed_bytes = 0
       @ready = false
       @body_remain = 0
@@ -190,18 +188,27 @@ def reset(fast_check=true)
         rescue IOError
           # swallow it
         end
-
       end
     end
 
     def close
+      tempfile_close
       begin
         @io.close
       rescue IOError, Errno::EBADF
         Puma::Util.purge_interrupt_queue
       end
     end
 
+    def tempfile_close
+      tf_path = @tempfile&.path
+      @tempfile&.close
+      File.unlink(tf_path) if tf_path
+      @tempfile = nil
+      @body = nil
+    rescue Errno::ENOENT, IOError
+    end
+
     # If necessary, read the PROXY protocol from the buffer. Returns
     # false if more data is needed.
     def try_to_parse_proxy_protocol
@@ -240,6 +247,7 @@ def try_to_finish
 
       return read_body if in_data_phase
 
+      data = nil
       begin
         data = @io.read_nonblock(CHUNK_SIZE)
       rescue IO::WaitReadable
@@ -428,8 +436,8 @@ def setup_body
       end
 
       if remain > MAX_BODY
-        @body = Tempfile.new(Const::PUMA_TMP_BASE)
-        @body.unlink
+        @body = Tempfile.create(Const::PUMA_TMP_BASE)
+        File.unlink @body.path unless IS_WINDOWS
         @body.binmode
         @tempfile = @body
       else
@@ -521,8 +529,8 @@ def setup_chunked_body(body)
       @prev_chunk = ""
       @excess_cr = 0
 
-      @body = Tempfile.new(Const::PUMA_TMP_BASE)
-      @body.unlink
+      @body = Tempfile.create(Const::PUMA_TMP_BASE)
+      File.unlink @body.path unless IS_WINDOWS
       @body.binmode
       @tempfile = @body
       @chunked_content_length = 0

lib/puma/request.rb

@@ -53,7 +53,6 @@ def handle_request(client, requests)
       socket  = client.io   # io may be a MiniSSL::Socket
       app_body = nil
 
-
       return false if closed_socket?(socket)
 
       if client.http_content_length_limit_exceeded
@@ -135,7 +134,7 @@ def handle_request(client, requests)
       io_buffer.reset
       uncork_socket client.io
       app_body.close if app_body.respond_to? :close
-      client.tempfile&.unlink
+      client&.tempfile_close
       after_reply = env[RACK_AFTER_REPLY] || []
       begin
         after_reply.each { |o| o.call }

test/test_puma_server.rb

@@ -912,6 +912,48 @@ def test_chunked_request
     assert_nil transfer_encoding
   end
 
+  # See also test_chunked_keep_alive_two_back_to_back
+  def test_two_back_to_back_chunked_have_different_tempfile
+    body = nil
+    content_length = nil
+    transfer_encoding = nil
+    req_body_path = nil
+    server_run { |env|
+      io = env['rack.input']
+      req_body_path = io.path
+      body = io.read
+      content_length = env['CONTENT_LENGTH']
+      transfer_encoding = env['HTTP_TRANSFER_ENCODING']
+      [200, {}, [""]]
+    }
+
+    chunked_req = "GET / HTTP/1.1\r\nTransfer-Encoding: gzip,chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n\r\n"
+
+    skt = new_connection
+
+    skt.syswrite chunked_req
+
+    response = skt.sysread 1_024
+    path1 = req_body_path
+
+    assert_equal "HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n", response
+    assert_equal "hello", body
+    assert_equal "5", content_length
+    assert_nil transfer_encoding
+
+    skt.syswrite chunked_req
+    response = skt.sysread 1_024
+    path2 = req_body_path
+
+    # same as above
+    assert_equal "HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n", response
+    assert_equal "hello", body
+    assert_equal "5", content_length
+    assert_nil transfer_encoding
+
+    refute_equal path1, path2
+  end
+
   def test_large_chunked_request
     body = nil
     content_length = nil
@@ -1879,6 +1921,8 @@ def test_form_data_encoding_windows_bom
 
     assert_equal file_bytesize, form_file_data.bytesize
     assert_equal out_r.read.bytesize, req_body.bytesize
+  ensure
+    File.unlink(temp_file_path) if File.exist? temp_file_path
   end
 
   def test_form_data_encoding_windows