From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 13 May 2021 18:52:20 GMT
Subject: Re: [PATCH 16/23] fsmonitor--daemon: implement handle_client callback
Message-ID: <0b9e54ba-ac27-e537-7bef-1b4448f92352@gmail.com>
In-Reply-To: <2b4ae4fc3d622f0fee8eb5e527a51467e13acfc5.1617291666.git.gitgitgadget@gmail.com>

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:

Here is a rather important bug that I found on a whim while working
with sparse-index integrations. The sparse-index isn't important
except that it caused a different pattern of batch creation and
responses from the daemon.

> +/*
> + * Format an opaque token string to send to the client.
> + */
> +static void fsmonitor_format_response_token(
> +	struct strbuf *response_token,
> +	const struct strbuf *response_token_id,
> +	const struct fsmonitor_batch *batch)
> +{
> +	uint64_t seq_nr = (batch) ? batch->batch_seq_nr + 1 : 0;

Here, you add one to the batch value to indicate a difference
between "zero" and "positive" values.

> +
> +	strbuf_reset(response_token);
> +	strbuf_addf(response_token, "builtin:%s:%"PRIu64,
> +		    response_token_id->buf, seq_nr);
> +}
> +
> +/*
> + * Parse an opaque token from the client.
> + */
> +static int fsmonitor_parse_client_token(const char *buf_token,
> +					struct strbuf *requested_token_id,
> +					uint64_t *seq_nr)
> +{
> +	const char *p;
> +	char *p_end;
> +
> +	strbuf_reset(requested_token_id);
> +	*seq_nr = 0;
> +
> +	if (!skip_prefix(buf_token, "builtin:", &p))
> +		return 1;
> +
> +	while (*p && *p != ':')
> +		strbuf_addch(requested_token_id, *p++);
> +	if (!*p++)
> +		return 1;
> +
> +	*seq_nr = (uint64_t)strtoumax(p, &p_end, 10);

Which means here you should decrement one from the value, possibly,
(except if it is zero).

> +	if (*p_end)
> +		return 1;
> +
> +	return 0;
> +}

...

> +	shown = kh_init_str();
> +	for (batch = batch_head;
> +	     batch && batch->batch_seq_nr >= requested_oldest_seq_nr;
> +	     batch = batch->next) {

And without either decrementing one from requested_oldest_seq_nr or
adding one to the batch_seq_nr here, this loop could terminate
immediately.

In my testing, I added one to the left-hand side of the inequality.

Thanks,
-Stolee