Re: [PATCH] reftable/writer: ensure valid range for log's update_index

[Patrick Steinhardt <ps@pks.im>]

On Thu, Dec 05, 2024 at 04:49:57PM +0100, Karthik Nayak wrote:
> Each reftable addition has an associated update_index. While writing
> refs, the update_index is verified to be within the range of the
> reftable writer, i.e. `writer.min_update_index < ref.update_index` and
> `writer.max_update_index > ref.update_index`.

These should probably be `<=` and `>=`, respectively.

> diff --git a/reftable/writer.c b/reftable/writer.c
> index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w,
>  	if (log->value_type == REFTABLE_LOG_DELETION)
>  		return reftable_writer_add_log_verbatim(w, log);
>  
> +	/*
> +	 * Verify only the upper limit of the update_index. Each reflog entry
> +	 * is tied to a specific update_index. Entries in the reflog can be
> +	 * replaced by adding a new entry with the same update_index,
> +	 * effectively canceling the old one.
> +	 *
> +	 * Consequently, reflog updates may include update_index values lower
> +	 * than the writer's min_update_index.
> +	 */
> +	if (log->update_index > w->max_update_index)
> +		return REFTABLE_API_ERROR;

Yup, looks sensible.

>  	if (!log->refname)
>  		return REFTABLE_API_ERROR;
>  
> diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
> index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644
> --- a/t/unit-tests/t-reftable-readwrite.c
> +++ b/t/unit-tests/t-reftable-readwrite.c
> @@ -151,6 +151,45 @@ static void t_log_overflow(void)
>  	reftable_buf_release(&buf);
>  }
>  
> +static void t_log_write_limits(void)
> +{
> +	struct reftable_write_options opts = { 0 };
> +	struct reftable_buf buf = REFTABLE_BUF_INIT;
> +	struct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);
> +	struct reftable_log_record log = {
> +		.refname = (char *)"refs/head/master",
> +		.update_index = 1,
> +		.value_type = REFTABLE_LOG_UPDATE,
> +		.value = {
> +			.update = {
> +				.old_hash = { 1 },
> +				.new_hash = { 2 },
> +				.name = (char *)"Han-Wen Nienhuys",
> +				.email = (char *)"hanwen@google.com",
> +				.tz_offset = 100,
> +				.time = 0x5e430672,
> +			},
> +		},
> +	};
> +	int err;
> +
> +	reftable_writer_set_limits(w, 1, 2);
> +
> +	err = reftable_writer_add_log(w, &log);
> +	check_int(err, ==, 0);
> +
> +	log.update_index = 2;
> +	err = reftable_writer_add_log(w, &log);
> +	check_int(err, ==, 0);
> +
> +	log.update_index = 3;
> +	err = reftable_writer_add_log(w, &log);
> +	check_int(err, ==, REFTABLE_API_ERROR);
> +
> +	reftable_writer_free(w);
> +	reftable_buf_release(&buf);
> +}

Makes sense, as well. We could trivially extend this test to also assert
that we can successfully write a log record with update index 0, which
would be smaller than the lower bound.

Patrick