From 9e8eaac52a977565f3a7b6a8ed688e0ec238a5df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Wed, 27 Oct 2021 16:06:37 +0200 Subject: [PATCH 1/8] Always send lowlevel_error response to client --- lib/puma/request.rb | 66 ++++++++++++++++++++---------------- lib/puma/server.rb | 20 ++++++----- test/test_response_header.rb | 4 +-- 3 files changed, 50 insertions(+), 40 deletions(-) diff --git a/lib/puma/request.rb b/lib/puma/request.rb index 948992895f..2d756657a8 100644 --- a/lib/puma/request.rb +++ b/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,36 +65,48 @@ 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) + status, headers, res_body = @thread_pool.with_force_shutdown do + @app.call(env) + end + + return :async if client.hijacked + + status = status.to_i + + if status == -1 + unless headers.empty? and res_body == [] + raise "async response must have empty headers and body" end - return :async if client.hijacked + return :async + 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 = status.to_i + status, headers, res_body = lowlevel_error(e, env, 500) + end - if status == -1 - unless headers.empty? and res_body == [] - raise "async response must have empty headers and body" - end + write_response(status, headers, res_body, lines, requests, client) + end - return :async - end - rescue ThreadPool::ForceShutdown => e - @events.unknown_error e, client, "Rack app" - @events.log "Detected force shutdown of a thread" + def write_response(status, headers, res_body, lines, requests, client) + env = client.env + io = client.io - status, headers, res_body = lowlevel_error(e, env, 503) - rescue Exception => e - @events.unknown_error e, client, "Rack app" + return false if closed_socket?(io) - status, headers, res_body = lowlevel_error(e, env, 500) - end + head = env[REQUEST_METHOD] == HEAD + after_reply = env[RACK_AFTER_REPLY] || [] + begin res_info = {} res_info[:content_length] = nil res_info[:no_body] = head @@ -149,9 +157,9 @@ def handle_request(client, lines, requests) 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 + 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 @@ -169,7 +177,7 @@ def handle_request(client, lines, requests) ensure uncork_socket io - body.close + client.body.close if client.body client.tempfile.unlink if client.tempfile res_body.close if res_body.respond_to? :close diff --git a/lib/puma/server.rb b/lib/puma/server.rb index aac89aecea..ce94da62e9 100644 --- a/lib/puma/server.rb +++ b/lib/puma/server.rb @@ -482,7 +482,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 @@ -510,34 +510,36 @@ 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 + return [handler_status || status, headers || {}, res_body || []] end if @leak_stack_on_error diff --git a/test/test_response_header.rb b/test/test_response_header.rb index 508bbcd78c..03fb43ca54 100644 --- a/test/test_response_header.rb +++ b/test/test_response_header.rb @@ -45,7 +45,7 @@ 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 @@ -53,7 +53,7 @@ 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 From a5d83c33fba7dff5285b8e58aab3683d6f3397b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Thu, 28 Oct 2021 08:22:37 +0200 Subject: [PATCH 2/8] Add spec for lowlever error handler --- test/test_puma_server.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index bfa2b977e3..01a171e93f 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -1362,4 +1362,21 @@ def test_rack_url_scheme_user data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" assert_equal "user", data.split("\r\n").last end + + # The server should send lowlevel_error handlers response to the client + def test_lowlevel_error + options = { lowlevel_error_handler: ->(err) { [200, {}, ["error page"]] } } + app = ->(env) { [200, nil, []] } + server_run(**options, &app) + + sock = send_http "GET / HTTP/1.0\r\n\r\n" + + _h = header sock + + body = sock.gets + + assert_match /error page/, body + + sock.close + end end From def464a5c98d7611407e5e6451e32a6620fb56eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Thu, 28 Oct 2021 11:07:02 +0200 Subject: [PATCH 3/8] Make sure we have a clean buffer when starting response --- lib/puma/request.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/puma/request.rb b/lib/puma/request.rb index 2d756657a8..b35fcbe20d 100644 --- a/lib/puma/request.rb +++ b/lib/puma/request.rb @@ -102,6 +102,7 @@ def write_response(status, headers, res_body, lines, requests, client) io = client.io return false if closed_socket?(io) + lines.clear head = env[REQUEST_METHOD] == HEAD after_reply = env[RACK_AFTER_REPLY] || [] From 7d53feffc52ad6f054bf438e7abd792e056ae30a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Thu, 28 Oct 2021 11:15:51 +0200 Subject: [PATCH 4/8] Simplify test --- test/test_puma_server.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index 01a171e93f..1c879c0407 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -1369,14 +1369,8 @@ def test_lowlevel_error app = ->(env) { [200, nil, []] } server_run(**options, &app) - sock = send_http "GET / HTTP/1.0\r\n\r\n" - - _h = header sock - - body = sock.gets - - assert_match /error page/, body + data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" - sock.close + assert_match /error page/, data end end From 9a96822fae3126069ce9c2986af5957505f3bf8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Thu, 28 Oct 2021 11:18:18 +0200 Subject: [PATCH 5/8] Rename spec --- test/test_puma_server.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index 1c879c0407..c328047145 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -1364,7 +1364,7 @@ def test_rack_url_scheme_user end # The server should send lowlevel_error handlers response to the client - def test_lowlevel_error + def test_lowlevel_error_handler options = { lowlevel_error_handler: ->(err) { [200, {}, ["error page"]] } } app = ->(env) { [200, nil, []] } server_run(**options, &app) From 9020b01a098eb52250d35c6db731892286f3c8a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Thu, 28 Oct 2021 11:29:34 +0200 Subject: [PATCH 6/8] Add method docs --- lib/puma/request.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/puma/request.rb b/lib/puma/request.rb index b35fcbe20d..8950cf282a 100644 --- a/lib/puma/request.rb +++ b/lib/puma/request.rb @@ -97,6 +97,15 @@ def handle_request(client, lines, requests) write_response(status, headers, res_body, lines, requests, client) end + # 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 From 40c72f904a4229633a4e31af1283f0f4d65ddb00 Mon Sep 17 00:00:00 2001 From: Patrik Ragnarsson Date: Thu, 28 Oct 2021 12:08:41 +0200 Subject: [PATCH 7/8] Tweak the test --- test/test_puma_server.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index c328047145..bfc82d9c29 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -363,6 +363,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) { [200, {}, ["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{HTTP/1.0 200 OK\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 @@ -1362,15 +1373,4 @@ def test_rack_url_scheme_user data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" assert_equal "user", data.split("\r\n").last end - - # The server should send lowlevel_error handlers response to the client - def test_lowlevel_error_handler - options = { lowlevel_error_handler: ->(err) { [200, {}, ["error page"]] } } - app = ->(env) { [200, nil, []] } - server_run(**options, &app) - - data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" - - assert_match /error page/, data - end end From 3bb0ace2fc36ecf8df1b391a8420c38d09d708ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A4lter?= Date: Tue, 2 Nov 2021 15:21:21 +0100 Subject: [PATCH 8/8] Return 500 from lowlevel_error_handler in test --- test/test_puma_server.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index bfc82d9c29..c70d8aa97e 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -364,14 +364,14 @@ def test_lowlevel_error_message end def test_lowlevel_error_handler_custom_response - options = { lowlevel_error_handler: ->(_err) { [200, {}, ["error page"]] } } + 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{HTTP/1.0 200 OK\r\nContent-Length: 10\r\n\r\nerror page}, data + assert_match %r{HTTP/1.0 500 Internal Server Error\r\nContent-Length: 10\r\n\r\nerror page}, data end def test_force_shutdown_error_default