Always send lowlevel_error response to client

lib/puma/request.rb

@@ -46,11 +46,7 @@ def handle_request(client, lines, requests)
       env[HIJACK_P] = true
       env[HIJACK] = client
 
-      body = client.body
-
-      head = env[REQUEST_METHOD] == HEAD
-
-      env[RACK_INPUT] = body
+      env[RACK_INPUT] = client.body
       env[RACK_URL_SCHEME] ||= default_server_port(env) == PORT_443 ? HTTPS : HTTP
 
       if @early_hints
@@ -69,118 +65,132 @@ def handle_request(client, lines, requests)
       # A rack extension. If the app writes #call'ables to this
       # array, we will invoke them when the request is done.
       #
-      after_reply = env[RACK_AFTER_REPLY] = []
+      env[RACK_AFTER_REPLY] = []
 
       begin
-        begin
-          status, headers, res_body = @thread_pool.with_force_shutdown do
-            @app.call(env)
-          end
-
-          return :async if client.hijacked
+        status, headers, res_body = @thread_pool.with_force_shutdown do
+          @app.call(env)
+        end
 
-          status = status.to_i
+        return :async if client.hijacked
 
-          if status == -1
-            unless headers.empty? and res_body == []
-              raise "async response must have empty headers and body"
-            end
+        status = status.to_i
 
-            return :async
+        if status == -1
+          unless headers.empty? and res_body == []
+            raise "async response must have empty headers and body"
           end
-        rescue ThreadPool::ForceShutdown => e
-          @events.unknown_error e, client, "Rack app"
-          @events.log "Detected force shutdown of a thread"
 
-          status, headers, res_body = lowlevel_error(e, env, 503)
-        rescue Exception => e
-          @events.unknown_error e, client, "Rack app"
-
-          status, headers, res_body = lowlevel_error(e, env, 500)
+          return :async
         end
+      rescue ThreadPool::ForceShutdown => e
+        @events.unknown_error e, client, "Rack app"
+        @events.log "Detected force shutdown of a thread"
 
-        res_info = {}
-        res_info[:content_length] = nil
-        res_info[:no_body] = head
+        status, headers, res_body = lowlevel_error(e, env, 503)
+      rescue Exception => e
+        @events.unknown_error e, client, "Rack app"
 
-        res_info[:content_length] = if res_body.kind_of? Array and res_body.size == 1
-          res_body[0].bytesize
-        else
-          nil
-        end
+        status, headers, res_body = lowlevel_error(e, env, 500)
+      end
 
-        cork_socket io
+      write_response(status, headers, res_body, lines, requests, client)
+    end
 
-        str_headers(env, status, headers, res_info, lines, requests, client)
+    # Does the actual response writing for Request#handle_request and Server#client_error
+    #
+    # @param status [Integer] the status returned by the Rack application
+    # @param headers [Hash] the headers returned by the Rack application
+    # @param res_body [Array] the body returned by the Rack application
+    # @param lines [Puma::IOBuffer] modified in place
+    # @param requests [Integer] number of inline requests handled
+    # @param client [Puma::Client]
+    # @return [Boolean,:async]
+    def write_response(status, headers, res_body, lines, requests, client)
+      env = client.env
+      io  = client.io
 
-        line_ending = LINE_END
+      head = env[REQUEST_METHOD] == HEAD
+      after_reply = env[RACK_AFTER_REPLY] ||= []
 
-        content_length  = res_info[:content_length]
-        if res_body && !res_body.respond_to?(:each)
-          response_hijack = res_body
-        else
-          response_hijack = res_info[:response_hijack]
-        end
+      res_info = {}
+      res_info[:no_body] = head
 
-        if res_info[:no_body]
-          if content_length and status != 204
-            lines.append CONTENT_LENGTH_S, content_length.to_s, line_ending
-          end
+      res_info[:content_length] = res_body.kind_of?(Array) && res_body.size == 1 ?
+        res_body[0].bytesize : nil
 
-          lines << LINE_END
-          fast_write io, lines.to_s
-          return res_info[:keep_alive]
-        end
+      cork_socket io
 
-        if content_length
+      str_headers(env, status, headers, res_info, lines, requests, client)
+
+      line_ending = LINE_END
+
+      content_length  = res_info[:content_length]
+
+      if res_body && !res_body.respond_to?(:each)
+        response_hijack = res_body
+      else
+        response_hijack = res_info[:response_hijack]
+      end
+
+      if res_info[:no_body]
+        if content_length and status != 204
           lines.append CONTENT_LENGTH_S, content_length.to_s, line_ending
-          chunked = false
-        elsif !response_hijack and res_info[:allow_chunked]
-          lines << TRANSFER_ENCODING_CHUNKED
-          chunked = true
         end
 
-        lines << line_ending
-
+        lines << LINE_END
         fast_write io, lines.to_s
+        return res_info[:keep_alive]
+      end
 
-        if response_hijack
-          response_hijack.call io
-          return :async
-        end
+      if content_length
+        lines.append CONTENT_LENGTH_S, content_length.to_s, line_ending
+        chunked = false
+      elsif !response_hijack and res_info[:allow_chunked]
+        lines << TRANSFER_ENCODING_CHUNKED
+        chunked = true
+      end
 
-        begin
-          res_body.each do |part|
-            next if part.bytesize.zero?
-            if chunked
-               fast_write io, (part.bytesize.to_s(16) << line_ending)
-               fast_write io, part            # part may have different encoding
-               fast_write io, line_ending
-            else
-              fast_write io, part
-            end
-            io.flush
-          end
+      lines << line_ending
+
+      fast_write io, lines.to_s
+
+      if response_hijack
+        response_hijack.call io
+        return :async
+      end
 
+      begin
+        res_body.each do |part|
+          next if part.bytesize.zero?
           if chunked
-            fast_write io, CLOSE_CHUNKED
-            io.flush
+            fast_write io, (part.bytesize.to_s(16) << line_ending)
+            fast_write io, part            # part may have different encoding
+            fast_write io, line_ending
+          else
+            fast_write io, part
           end
-        rescue SystemCallError, IOError
-          raise ConnectionError, "Connection error detected during write"
+          io.flush
         end
 
-      ensure
-        uncork_socket io
-
-        body.close
-        client.tempfile.unlink if client.tempfile
-        res_body.close if res_body.respond_to? :close
-
-        after_reply.each { |o| o.call }
+        if chunked
+          fast_write io, CLOSE_CHUNKED
+          io.flush
+        end
+      rescue SystemCallError, IOError
+        raise ConnectionError, "Connection error detected during write"
       end
-
       res_info[:keep_alive]
+
+    ensure
+      uncork_socket io
+      # lines must be reset if an error is raised
+      lines.reset
+      client.body.close if client.body
+      client.tempfile.unlink if client.tempfile
+      res_body.close if res_body.respond_to? :close
+
+      after_reply.each { |o| o.call }
     end
 
     # @param env [Hash] see Puma::Client#env, from request

lib/puma/server.rb

@@ -476,7 +476,7 @@ def process_client(client, buffer)
         end
         true
       rescue StandardError => e
-        client_error(e, client)
+        client_error(e, client, buffer, requests)
         # The ensure tries to close +client+ down
         requests > 0
       ensure
@@ -504,37 +504,37 @@ def with_force_shutdown(client, &block)
     # :nocov:
 
     # Handle various error types thrown by Client I/O operations.
-    def client_error(e, client)
+    def client_error(e, client, buffer = ::Puma::IOBuffer.new, requests = 1)
       # Swallow, do not log
       return if [ConnectionError, EOFError].include?(e.class)
 
-      lowlevel_error(e, client.env)
       case e
       when MiniSSL::SSLError
         @events.ssl_error e, client.io
       when HttpParserError
-        client.write_error(400)
+        status, headers, res_body = lowlevel_error(e, client.env, 400)
+        write_response(status, headers, res_body, buffer, requests, client)
         @events.parse_error e, client
       else
-        client.write_error(500)
+        status, headers, res_body = lowlevel_error(e, client.env)
+        write_response(status, headers, res_body, buffer, requests, client)
         @events.unknown_error e, nil, "Read"
       end
     end
 
     # A fallback rack response if +@app+ raises as exception.
     #
-    def lowlevel_error(e, env, status=500)
+    def lowlevel_error(e, env, status = 500)
       if handler = @options[:lowlevel_error_handler]
         if handler.arity == 1
-          return handler.call(e)
+          handler_status, headers, res_body = handler.call(e)
         elsif handler.arity == 2
-          return handler.call(e, env)
+          handler_status, headers, res_body = handler.call(e, env)
         else
-          return handler.call(e, env, status)
+          handler_status, headers, res_body = handler.call(e, env, status)
         end
-      end
-
-      if @leak_stack_on_error
+        [handler_status || status, headers || {}, res_body || []]
+      elsif @leak_stack_on_error
         backtrace = e.backtrace.nil? ? '<no backtrace available>' : e.backtrace.join("\n")
         [status, {}, ["Puma caught this error: #{e.message} (#{e.class})\n#{backtrace}"]]
       else

test/test_puma_server.rb

@@ -383,6 +383,17 @@ def test_lowlevel_error_message
     assert (data.size > 0), "Expected response message to be not empty"
   end
 
+  def test_lowlevel_error_handler_custom_response
+    options = { lowlevel_error_handler: ->(_err) { [500, {}, ["error page"]] } }
+    # setting the headers argument to nil will trigger exception inside Puma
+    broken_app = ->(_env) { [200, nil, []] }
+    server_run(**options, &broken_app)
+
+    data = send_http_and_read "GET / HTTP/1.0\r\n\r\n"
+
+    assert_match %r{\AHTTP/1.0 500 Internal Server Error\r\nContent-Length: 10\r\n\r\nerror page}, data
+  end
+
   def test_force_shutdown_error_default
     server_run(force_shutdown_after: 2) do
       @server.stop

test/test_response_header.rb

@@ -45,15 +45,15 @@ def test_integer_key
     server_run app: ->(env) { [200, { 1 => 'Boo'}, []] }
     data = send_http_and_read "GET / HTTP/1.0\r\n\r\n"
 
-    assert_match(/HTTP\/1.1 500 Internal Server Error/, data)
+    assert_match(/HTTP\/1.0 500 Internal Server Error/, data)
   end
 
   # The header must respond to each
   def test_nil_header
     server_run app: ->(env) { [200, nil, []] }
     data = send_http_and_read "GET / HTTP/1.0\r\n\r\n"
 
-    assert_match(/HTTP\/1.1 500 Internal Server Error/, data)
+    assert_match(/HTTP\/1.0 500 Internal Server Error/, data)
   end
 
   # The values of the header must be Strings