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
