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

> 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