Skip to content

Commit

Permalink
parser: commit after parsing each header line (#54)
Browse files Browse the repository at this point in the history
Upstream issue inhabitedtype/httpaf#18 details
a case where the http/af parser is unable to parse a message if the read
buffer size isn't as big as all the bytes that form the message headers.
While it is desirable, and a design goal of the library, to be efficient
in parsing and thus avoid DoS attacks, it is more sensible to allow each
message header to fit within the read buffer size up to its limit. This
diff makes that change, which better accommodates real world use cases
for the default read buffer size.
  • Loading branch information
anmonteiro committed May 11, 2020
1 parent 75f2d7b commit 346a587
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 1 deletion.
1 change: 1 addition & 0 deletions lib/parse.ml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ let header =
lift2 (fun key value -> (key, value))
(take_till P.is_space_or_colon <* char ':' <* spaces)
(take_till P.is_cr <* eol >>| String.trim)
<* commit
<?> "header"

let headers =
Expand Down
47 changes: 46 additions & 1 deletion lib_test/test_client_connection.ml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,50 @@ let default_response_handler expected_response response body =

let no_error_handler _ = assert false

let test_commit_parse_after_every_header () =
let request' = Request.create `GET "/" in
let response =
Response.create
~headers:(Headers.of_list
[ "Links", "/path/to/some/website"
; "Links", "/path/to/some/website"
; "connection", "close"
])
`OK
in

let t = create ?config:None in
let body =
request
t
request'
~response_handler:(default_response_handler response)
~error_handler:no_error_handler
in
Body.close_writer body;
write_request t request';

let response_line = "HTTP/1.1 200 OK\r\n" in
let single_header = "Links: /path/to/some/website\r\n" in
let r =
(* Each header is 30 bytes *)
response_line ^ single_header ^ single_header ^ "connection: close\r\n\r\n"
in
let bs = Bigstringaf.of_string r ~off:0 ~len:(String.length r) in
let c = read t bs ~off:0 ~len:(String.length response_line + 15) in
Alcotest.(check int) "only reads the response line" (String.length response_line) c;

let c' =
read t bs ~off:c ~len:(String.length single_header)
in
Alcotest.(check int) "parser can read a single header and commit" (String.length single_header) c';

let c'' = read_eof t bs ~off:(c + c') ~len:(String.length r - (c + c')) in
Alcotest.(check int) "read_eof with the rest of the input is accepted" (String.length r - (c + c')) c'';

connection_is_shutdown t;
;;

let test_get () =
let request' = Request.create `GET "/" in
let response = Response.create `OK in
Expand Down Expand Up @@ -580,7 +624,8 @@ let test_client_upgrade () =
;;

let tests =
[ "GET" , `Quick, test_get
[ "commit parse after every header line", `Quick, test_commit_parse_after_every_header
; "GET" , `Quick, test_get
; "Response EOF", `Quick, test_response_eof
; "Response header order preserved", `Quick, test_response_header_order
; "report_exn" , `Quick, test_report_exn
Expand Down
24 changes: 24 additions & 0 deletions lib_test/test_server_connection.ml
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,29 @@ let test_reader_is_closed_after_eof () =
connection_is_shutdown t;
;;

let test_commit_parse_after_every_header () =
let t = create default_request_handler in
let request_line = "GET / HTTP/1.1\r\n" in
let single_header = "Links: /path/to/some/website\r\n" in
let r =
(* Each header is 30 bytes *)
request_line ^ single_header ^ single_header ^ "connection: close\r\n\r\n"
in
let bs = Bigstringaf.of_string r ~off:0 ~len:(String.length r) in
let c = read t bs ~off:0 ~len:30 in
Alcotest.(check int) "only reads the request line" (String.length request_line) c;

let c' =
read t bs ~off:c ~len:(String.length single_header)
in
Alcotest.(check int) "parser can read a single header and commit" (String.length single_header) c';

let c'' = read_eof t bs ~off:(c + c') ~len:(String.length r - (c + c')) in
Alcotest.(check int) "read_eof with the rest of the input is accepted" (String.length r - (c + c')) c'';
write_response t (Response.create `OK);
connection_is_shutdown t;
;;

let test_single_get () =
(* Single GET *)
let t = create default_request_handler in
Expand Down Expand Up @@ -748,6 +771,7 @@ let test_yield_before_starting_a_response () =
let tests =
[ "initial reader state" , `Quick, test_initial_reader_state
; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof
; "commit parse after every header line", `Quick, test_commit_parse_after_every_header
; "single GET" , `Quick, test_single_get
; "multiple GETs" , `Quick, test_multiple_get
; "asynchronous response" , `Quick, test_asynchronous_response
Expand Down

0 comments on commit 346a587

Please sign in to comment.