From fb2a7cbbb7fd933553abcd121f9ffdd4db53be69 Mon Sep 17 00:00:00 2001 From: David House Date: Thu, 29 Apr 2021 12:52:21 +0100 Subject: [PATCH 1/5] Fix parse error at EOF. Currently, if there is a parse error after [read_eof], the runtime has no chance to discover it. The error is not surfaced immediately, and if the runtime calls [next_read_operation], it just gets [`Close]. This is because [Reader.next] first checks to see if it is closed, and if so, always returns [`Close]. This feature fixes this by inverting the logic in that function. Unfortunately this leads to another problem: the parser effectively expects an unending stream of requests, because after finishing parsing one request it immediately expects the next one. This means that read_eof is guaranteed to lead to a parse error. Thankfully this is easily fixed by treating an empty input as a valid as well. --- lib/parse.ml | 37 +++++++++++++++--------------- lib_test/test_server_connection.ml | 19 +++++++++++++++ 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/lib/parse.ml b/lib/parse.ml index 664e3c3b..019d739d 100644 --- a/lib/parse.ml +++ b/lib/parse.ml @@ -239,16 +239,19 @@ module Reader = struct let request handler = let parser = - request <* commit >>= fun request -> - match Request.body_length request with - | `Error `Bad_request -> return (Error (`Bad_request request)) - | `Fixed 0L -> - handler request Body.empty; - ok - | `Fixed _ | `Chunked as encoding -> - let request_body = Body.create_reader Bigstringaf.empty in - handler request request_body; - body ~encoding request_body *> ok + at_end_of_input >>= function + | true -> ok + | false -> + request <* commit >>= fun request -> + match Request.body_length request with + | `Error `Bad_request -> return (Error (`Bad_request request)) + | `Fixed 0L -> + handler request Body.empty; + ok + | `Fixed _ | `Chunked as encoding -> + let request_body = Body.create_reader Bigstringaf.empty in + handler request request_body; + body ~encoding request_body *> ok in create parser @@ -322,13 +325,11 @@ module Reader = struct ;; let next t = - if t.closed - then `Close - else ( - match t.parse_state with - | Fail err -> `Error err - | Done -> `Read - | Partial _ -> `Read - ) + match t.parse_state with + | Fail err -> `Error err + | Done | Partial _ -> + if t.closed + then `Close + else `Read ;; end diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 056f85dd..2f711f3a 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -888,6 +888,24 @@ let test_parse_failure_after_checkpoint () = | Some error -> Alcotest.(check request_error) "Error" error `Bad_request ;; +let test_parse_failure_at_eof () = + let error_queue = ref None in + let error_handler ?request:_ error _start_response = + Alcotest.(check (option reject)) "Error queue is empty" !error_queue None; + error_queue := Some error + in + let request_handler _reqd = assert false in + let t = create ~error_handler request_handler in + reader_ready t; + read_string t "GET index.html HTTP/1.1\r\n"; + let result = feed_string ~eof:true t " index.html HTTP/1.1\r\n\r\n" in + Alcotest.(check int) "Bad header not consumed" result 0; + reader_closed t; + match !error_queue with + | None -> Alcotest.fail "Expected error" + | Some error -> Alcotest.(check request_error) "Error" error `Bad_request +;; + let test_response_finished_before_body_read () = let response = Response.create `OK ~headers:(Headers.encoding_fixed 4) in let rev_body_chunks = ref [] in @@ -971,6 +989,7 @@ let tests = ; "multiple requests with connection close", `Quick, test_multiple_requests_in_single_read_with_close ; "multiple requests with eof", `Quick, test_multiple_requests_in_single_read_with_eof ; "parse failure after checkpoint", `Quick, test_parse_failure_after_checkpoint + ; "parse failure at eof", `Quick, test_parse_failure_at_eof ; "response finished before body read", `Quick, test_response_finished_before_body_read ; "shutdown in request handler", `Quick, test_shutdown_in_request_handler ; "shutdown during asynchronous request", `Quick, test_shutdown_during_asynchronous_request From 706c081108e5c7c878b368d976783022bb187e61 Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Wed, 2 Jun 2021 18:42:14 -0400 Subject: [PATCH 2/5] surface-parse-errors-at-eof: revert parser Instead of adding complexity to the parser, it seems better to me to simply not invoke `start` when we have no bytes to feed to the parser. The tests still pass with this. --- lib/parse.ml | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/parse.ml b/lib/parse.ml index 019d739d..a21f48cd 100644 --- a/lib/parse.ml +++ b/lib/parse.ml @@ -239,19 +239,16 @@ module Reader = struct let request handler = let parser = - at_end_of_input >>= function - | true -> ok - | false -> - request <* commit >>= fun request -> - match Request.body_length request with - | `Error `Bad_request -> return (Error (`Bad_request request)) - | `Fixed 0L -> - handler request Body.empty; - ok - | `Fixed _ | `Chunked as encoding -> - let request_body = Body.create_reader Bigstringaf.empty in - handler request request_body; - body ~encoding request_body *> ok + request <* commit >>= fun request -> + match Request.body_length request with + | `Error `Bad_request -> return (Error (`Bad_request request)) + | `Fixed 0L -> + handler request Body.empty; + ok + | `Fixed _ | `Chunked as encoding -> + let request_body = Body.create_reader Bigstringaf.empty in + handler request request_body; + body ~encoding request_body *> ok in create parser @@ -307,6 +304,7 @@ module Reader = struct let consumed = match t.parse_state with | Fail _ -> 0 + | Done when len = 0 -> 0 | Done -> start t (AU.parse t.parser); read_with_more t bs ~off ~len more; From 92a68e5552c2bbe4f0d94cff464f803abd0251ee Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Wed, 2 Jun 2021 18:45:40 -0400 Subject: [PATCH 3/5] demonstrate writing to closed writer --- lib_test/test_server_connection.ml | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 2f711f3a..802462c6 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -890,9 +890,15 @@ let test_parse_failure_after_checkpoint () = let test_parse_failure_at_eof () = let error_queue = ref None in - let error_handler ?request:_ error _start_response = + let continue = ref (fun () -> ()) in + let error_handler ?request error start_response = Alcotest.(check (option reject)) "Error queue is empty" !error_queue None; - error_queue := Some error + Alcotest.(check (option reject)) "Request was not parsed" request None; + error_queue := Some error; + continue := (fun () -> + let resp_body = start_response Headers.empty in + Body.write_string resp_body "got an error"; + Body.close_writer resp_body); in let request_handler _reqd = assert false in let t = create ~error_handler request_handler in @@ -901,9 +907,10 @@ let test_parse_failure_at_eof () = let result = feed_string ~eof:true t " index.html HTTP/1.1\r\n\r\n" in Alcotest.(check int) "Bad header not consumed" result 0; reader_closed t; - match !error_queue with - | None -> Alcotest.fail "Expected error" - | Some error -> Alcotest.(check request_error) "Error" error `Bad_request + (match !error_queue with + | None -> Alcotest.fail "Expected error" + | Some error -> Alcotest.(check request_error) "Error" error `Bad_request); + !continue () ;; let test_response_finished_before_body_read () = From ae36c3bac0f13f90a561d21cc9a7b1a19503d23a Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Sun, 6 Jun 2021 13:22:08 -0400 Subject: [PATCH 4/5] accept handler exception in test For now let's just accept that this is possible, since I believe it is in other situations already. --- lib/parse.ml | 1 + lib_test/test_server_connection.ml | 13 ++++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/parse.ml b/lib/parse.ml index a21f48cd..0c71ca2b 100644 --- a/lib/parse.ml +++ b/lib/parse.ml @@ -304,6 +304,7 @@ module Reader = struct let consumed = match t.parse_state with | Fail _ -> 0 + (* Don't feed empty input when we're at a request boundary *) | Done when len = 0 -> 0 | Done -> start t (AU.parse t.parser); diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index dfca6d67..9ed36a34 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -263,6 +263,12 @@ let connection_is_shutdown t = writer_closed t; ;; +let raises_writer_closed f = + (* This is raised when you write to a closed [Faraday.t] *) + Alcotest.check_raises "raises because writer is closed" + (Failure "cannot write to closed writer") f +;; + let request_handler_with_body body reqd = Body.close_reader (Reqd.request_body reqd); Reqd.respond_with_string reqd (Response.create `OK) body @@ -910,7 +916,7 @@ let test_parse_failure_at_eof () = (match !error_queue with | None -> Alcotest.fail "Expected error" | Some error -> Alcotest.(check request_error) "Error" error `Bad_request); - !continue () + raises_writer_closed !continue ;; let test_response_finished_before_body_read () = @@ -960,10 +966,7 @@ let test_shutdown_during_asynchronous_request () = in read_request t request; shutdown t; - (* This is raised from Faraday *) - Alcotest.check_raises "[continue] raises because writer is closed" - (Failure "cannot write to closed writer") - !continue; + raises_writer_closed !continue; reader_closed t; writer_closed t ;; From bf8a6cfeeed2b83acca0c10574e8224b8bed6088 Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Sun, 6 Jun 2021 14:25:57 -0400 Subject: [PATCH 5/5] allow error_handler to respond before shutdown The error handler in the latest test no longer raises that the writer is closed. There are a few things going on in this fix: - In `Reader.read_with_more`, we closed the reader when something was fed when `more = Complete`. This isn't entirely accurate, though -- it meant that a single `read_eof` with multiple requests would enter this branch multiple times because `Server_connection.read_with_more` was recursive and might call into the reader with the same buffer multiple times. Now we only close the reader once we've actually exhausted the bytes and hit eof. - In `_next_read_operation`, the first thing we do is check if we have no request and the reader is closed. In that case, we should be safe to shutdown the connection. One minor note is that this was really equivalent to `shutdown_writer` since we are basically testing that the reader is shut down. But the more complicated thing is that just because the `request_queue` is empty doesn't mean there isn't pending handler work -- specifically, when the `error_handler` is invoked without a `?request`. This happens during connection and parse errors, so we should make sure to keep the writer open until that handler is resolved. Minor changes: - `is_waiting` was unused - Added more tracing that I found helpful while debugging tests --- lib/parse.ml | 4 ++-- lib/server_connection.ml | 17 +++++++++---- lib_test/test_server_connection.ml | 38 ++++++++++++++++++++++-------- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/lib/parse.ml b/lib/parse.ml index 0c71ca2b..99313dcb 100644 --- a/lib/parse.ml +++ b/lib/parse.ml @@ -313,8 +313,8 @@ module Reader = struct transition t (continue bs more ~off ~len) in begin match more with - | Complete -> t.closed <- true; - | Incomplete -> () + | Complete when consumed = len -> t.closed <- true; + | Complete | Incomplete -> () end; consumed; ;; diff --git a/lib/server_connection.ml b/lib/server_connection.ml index c4b3680e..136465e1 100644 --- a/lib/server_connection.ml +++ b/lib/server_connection.ml @@ -64,15 +64,15 @@ type t = ; request_queue : Reqd.t Queue.t (* invariant: If [request_queue] is not empty, then the head of the queue has already had [request_handler] called on it. *) + ; mutable is_errored : bool + (* if there is a parse or connection error, we invoke the [error_handler] + and set [is_errored] to indicate we should not close the writer yet. *) ; mutable wakeup_reader : Optional_thunk.t } let is_closed t = Reader.is_closed t.reader && Writer.is_closed t.writer -let is_waiting t = - not (is_closed t) && Queue.is_empty t.request_queue - let is_active t = not (Queue.is_empty t.request_queue) @@ -134,6 +134,7 @@ let create ?(config=Config.default) ?(error_handler=default_error_handler) reque ; request_handler = request_handler ; error_handler = error_handler ; request_queue + ; is_errored = false ; wakeup_reader = Optional_thunk.none } @@ -166,6 +167,7 @@ let set_error_and_handle ?request t error = let reqd = current_reqd_exn t in Reqd.report_error reqd error end else begin + t.is_errored <- true; let status = match (error :> [error | Status.standard]) with | `Exn _ -> `Internal_server_error @@ -191,8 +193,11 @@ let advance_request_queue t = let rec _next_read_operation t = if not (is_active t) then ( - if Reader.is_closed t.reader - then shutdown t; + (* If the request queue is empty, there is no connection error, and the + reader is closed, then we can assume that no more user code will be able + to write. *) + if Reader.is_closed t.reader && not t.is_errored + then shutdown_writer t; Reader.next t.reader ) else ( let reqd = current_reqd_exn t in @@ -289,6 +294,8 @@ and _final_write_operation_for t reqd = _next_write_operation t; ) in + (* The only reason the reader yields is to wait for the writer, so we need to + notify it that we've completed. *) wakeup_reader t; next ;; diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 9ed36a34..3c049fa3 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -1,7 +1,7 @@ open Httpaf open Helpers -let trace fmt = Format.ksprintf (Format.printf "%s\n") fmt +let trace fmt = Format.ksprintf (Format.printf "%s\n%!") fmt let request_error_pp_hum fmt = function | `Bad_request -> Format.fprintf fmt "Bad_request" @@ -94,6 +94,15 @@ end = struct ;; let create ?config ?error_handler request_handler = + let request_handler r = + trace "invoked: request_handler"; + request_handler r + in + let error_handler = + Option.map (fun error_handler ?request -> + trace "invoked: request_handler"; + error_handler ?request) error_handler + in let rec t = lazy ( { server_connection = create ?config ?error_handler request_handler @@ -126,23 +135,27 @@ end = struct let do_read t f = match current_read_operation t with | `Read -> + trace "read: start"; let res = f t.server_connection in + trace "read: finished"; t.read_loop (); res | `Yield | `Close as op -> - Alcotest.failf "Read attempted during operation: %a" - Read_operation.pp_hum op + Alcotest.failf "Read attempted during operation: %a" + Read_operation.pp_hum op ;; let do_write t f = match current_write_operation t with | `Write bufs -> - let res = f t.server_connection bufs in - t.write_loop (); - res + trace "write: start"; + let res = f t.server_connection bufs in + trace "write: finished"; + t.write_loop (); + res | `Yield | `Close _ as op -> - Alcotest.failf "Write attempted during operation: %a" - Write_operation.pp_hum op + Alcotest.failf "Write attempted during operation: %a" + Write_operation.pp_hum op ;; let on_reader_unyield t f = @@ -285,7 +298,10 @@ let echo_handler response reqd = Body.write_string response_body (Bigstringaf.substring ~off ~len buffer); Body.flush response_body (fun () -> Body.schedule_read request_body ~on_eof ~on_read) - and on_eof () = print_endline "got eof"; Body.close_writer response_body in + and on_eof () = + print_endline "echo handler eof"; + Body.close_writer response_body + in Body.schedule_read request_body ~on_eof ~on_read; ;; @@ -916,7 +932,9 @@ let test_parse_failure_at_eof () = (match !error_queue with | None -> Alcotest.fail "Expected error" | Some error -> Alcotest.(check request_error) "Error" error `Bad_request); - raises_writer_closed !continue + !continue (); + write_response t (Response.create `Bad_request) ~body:"got an error"; + writer_closed t; ;; let test_response_finished_before_body_read () =