Re: [PATCH v6 06/12] test-http-server: add HTTP request parsing

From: Matthew John Cheetham <mjcheetham@outlook.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Lessley Dennington <lessleydennington@gmail.com>,
	M Hickford <mirth.hickford@gmail.com>,
	Jeff Hostetler <git@jeffhostetler.com>,
	Glen Choo <chooglen@google.com>, Victoria Dye <vdye@github.com>
Subject: Re: [PATCH v6 06/12] test-http-server: add HTTP request parsing
Date: Fri, 20 Jan 2023 14:05:28 -0800	[thread overview]
Message-ID: <AS2PR03MB98154B064352CEDF76E2924FC0C59@AS2PR03MB9815.eurprd03.prod.outlook.com> (raw)
In-Reply-To: <230118.86bkmwxf6e.gmgdl@evledraar.gmail.com>

On 2023-01-18 03:14, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> 
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Add ability to parse HTTP requests to the test-http-server test helper.
>>
>> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
>> ---
>>  t/helper/test-http-server.c | 175 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 173 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
>> index 6cdac223a55..36f4a54fe6d 100644
>> --- a/t/helper/test-http-server.c
>> +++ b/t/helper/test-http-server.c
>> @@ -83,6 +83,42 @@ enum worker_result {
>>  	WR_HANGUP   = 1<<1,
>>  };
>>  
>> +/*
>> + * Fields from a parsed HTTP request.
>> + */
>> +struct req {
>> +	struct strbuf start_line;
>> +
>> +	const char *method;
>> +	const char *http_version;
>> +
>> +	struct strbuf uri_path;
>> +	struct strbuf query_args;
>> +
>> +	struct string_list header_list;
>> +	const char *content_type;
>> +	ssize_t content_length;
>> +};
>> +
>> +#define REQ__INIT { \
>> +	.start_line = STRBUF_INIT, \
>> +	.uri_path = STRBUF_INIT, \
>> +	.query_args = STRBUF_INIT, \
>> +	.header_list = STRING_LIST_INIT_NODUP, \
>> +	.content_type = NULL, \
>> +	.content_length = -1 \
>> +	}
> 
> Style nit: Don't indent the trailing "}", and add a "," after the last
> "content_length" item.
> 
> We omit the comma by convention when there really should not be another
> item, such as when we have a "NULL" terminator, here though we might add
> a struct element at the end, so...

Sure.

>> +static enum worker_result req__read(struct req *req, int fd)
>> +{
>> +	struct strbuf h = STRBUF_INIT;
>> +	struct string_list start_line_fields = STRING_LIST_INIT_DUP;
>> +	int nr_start_line_fields;
>> +	const char *uri_target;
>> +	const char *query;
>> +	char *hp;
>> +	const char *hv;
>> +
>> +	enum worker_result result = WR_OK;
>> +
>> +	/*
>> +	 * Read line 0 of the request and split it into component parts:
>> +	 *
>> +	 *    <method> SP <uri-target> SP <HTTP-version> CRLF
>> +	 *
>> +	 */
>> +	if (strbuf_getwholeline_fd(&req->start_line, fd, '\n') == EOF) {
>> +		result = WR_OK | WR_HANGUP;
>> +		goto done;
>> +	}
>> +
>> +	strbuf_trim_trailing_newline(&req->start_line);
>> +
>> +	nr_start_line_fields = string_list_split(&start_line_fields,
>> +						 req->start_line.buf,
>> +						 ' ', -1);
>> +	if (nr_start_line_fields != 3) {
>> +		logerror("could not parse request start-line '%s'",
>> +			 req->start_line.buf);
>> +		result = WR_IO_ERROR;
>> +		goto done;
>> +	}
>> +
>> +	req->method = xstrdup(start_line_fields.items[0].string);
>> +	req->http_version = xstrdup(start_line_fields.items[2].string);
>> +
>> +	uri_target = start_line_fields.items[1].string;
>> +
>> +	if (strcmp(req->http_version, "HTTP/1.1")) {
>> +		logerror("unsupported version '%s' (expecting HTTP/1.1)",
>> +			 req->http_version);
>> +		result = WR_IO_ERROR;
>> +		goto done;
>> +	}
>> +
>> +	query = strchr(uri_target, '?');
>> +
>> +	if (query) {
>> +		strbuf_add(&req->uri_path, uri_target, (query - uri_target));
>> +		strbuf_trim_trailing_dir_sep(&req->uri_path);
>> +		strbuf_addstr(&req->query_args, query + 1);
>> +	} else {
>> +		strbuf_addstr(&req->uri_path, uri_target);
>> +		strbuf_trim_trailing_dir_sep(&req->uri_path);
>> +	}
>> +
>> +	/*
>> +	 * Read the set of HTTP headers into a string-list.
>> +	 */
>> +	while (1) {
>> +		if (strbuf_getwholeline_fd(&h, fd, '\n') == EOF)
>> +			goto done;
>> +		strbuf_trim_trailing_newline(&h);
>> +
>> +		if (!h.len)
>> +			goto done; /* a blank line ends the header */
>> +
>> +		hp = strbuf_detach(&h, NULL);
>> +		string_list_append(&req->header_list, hp);
>> +
>> +		/* also store common request headers as struct req members */
>> +		if (skip_prefix(hp, "Content-Type: ", &hv)) {
>> +			req->content_type = hv;
>> +		} else if (skip_prefix(hp, "Content-Length: ", &hv)) {
>> +			req->content_length = strtol(hv, &hp, 10);
> 
> In POSIX the "ssize_t" is not a "this is the unsigned size_t", but can
> be a much smaller integer type (although in practice it tends to be the
> signed version of "size_t".
> 
> But this seems like a potential overflow trap as a result, but sometimes
> we need to live with "ssize_t".
> 
> However, in this case it seems like we don't, as it seems the only
> reason you init'd this to -1 and then...
> 
>> +	if (trace2_is_enabled()) {
>> +		struct string_list_item *item;
>> +		trace2_printf("%s: %s", TR2_CAT, req->start_line.buf);
>> +		trace2_printf("%s: hver: %s", TR2_CAT, req->http_version);
>> +		trace2_printf("%s: hmth: %s", TR2_CAT, req->method);
>> +		trace2_printf("%s: path: %s", TR2_CAT, req->uri_path.buf);
>> +		trace2_printf("%s: qury: %s", TR2_CAT, req->query_args.buf);
>> +		if (req->content_length >= 0)
>> +			trace2_printf("%s: clen: %d", TR2_CAT, req->content_length);
> 
> ...use that ">= 0" is to keep the state of "did I assign to this above?
> 
> So firstly, shouldn't we error or something on a "Content-Length: 0",
> and aside from that wouldn't we just have a "int have_content_length =
> 0" in this function that we'd then flip to 1?

It seems like the perfect type for such a non-zero-or-error size value; from
POSIX specifications[1]:

> ...
> size_t
>	Used for sizes of objects.
> ssize_t
>	Used for a count of bytes or an error indication.
> ...

But you're probably right here that `ssize_t` isn't that suitable in practice
due to the comically low minimum size of the `SSIZE_MAX` (2^15 I believe).

RFC 9110 §8.6 [2] addresses the `Content-Length` HTTP header and says that its
value should be non-negative, but also have no upper bound; we're gonna have to
set at least some practical limit.

Libcurl handles this by writing it's own parsing function that's good up to
a max 64-bit integer value [3][4].

Given this is for a test helper and only going to be receiving data from tests,
I propose just using something like `uintmax_t` and storing a bit with
`unsigned has_content_length:1;` to show if we actually got a header in the
request or not.

Thanks,
Matthew

[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
[2] https://www.rfc-editor.org/rfc/rfc9110.html#section-8.6
[3] https://github.com/curl/curl/blob/6113dec2a829d4ab766428ccca9535b7a5efd012/lib/http.c#L3348-L3349
[4] https://github.com/curl/curl/blob/6113dec2a829d4ab766428ccca9535b7a5efd012/lib/strtoofft.c#L214-L218