Re: [PATCH 3/3] reftable: prevent 'update_index' changes after header write

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 20, 2025, 11:47 UTC
Message-ID: <CAOLa=ZQpg+8vg+3XMcG4XyoaOMvtMV7KuSkQhJXTkFiPatO7Lw@mail.gmail.com>
In-Reply-To: <Z4oi55XewgX2deTm@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 21 quoted lines

> On Fri, Jan 17, 2025 at 08:59:14AM +0100, Karthik Nayak wrote:
>> diff --git a/reftable/writer.c b/reftable/writer.c
>> index 740c98038eaf883258bef4988f78977ac7e4a75a..c602b873543790e36178f797ed9f98112671f97f 100644
>> --- a/reftable/writer.c
>> +++ b/reftable/writer.c
>> @@ -182,6 +182,13 @@ int reftable_writer_new(struct reftable_writer **out,
>>  void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
>>  				uint64_t max)
>>  {
>> +	/*
>> +	 * The limits shouldn't be modified post writing the first block, else
>> +	 * it would cause a mismatch between the header and the footer.
>> +	 */
>
> Can we make this *even* stricter? I think that this is something that is
> easy to do wrong, and the fact that it only triggers in some situations
> of misuse may easily make tests miss this issue. So ideally, we should
> assert that `set_limits()` is always called before queueing any records
> to the writer. This would make us error out in all situations where the
> calling order is wrong.
>

I agree here, it makes sense to make this stricter. Like you mentioned, currently they are independent. The only way to enforce the limits is to ensure that they are dependent.

Show 5 quoted lines

> There are two ways I can see us doing that:
>
>   - Detect any state written by `writer_add_record()` and error out if
>     it's set when `reftable_writer_set_limits()` is called.
>

Yeah I think this would be simple to do. I guess we can check `w->last_key` is set, since any record write would modify that.

>   - Adapt `reftable_writer_new()` so that it takes the update indices as
>     input and drop `reftable_writer_set_limits()` altogether.
>

This one is a bit harder to do because of our flow. Generally the writer is provided to callers via a callback function passed to `reftable_addition_add()`. I guess I could simply pass the data:

  caller -> reftable_addition_add() -> reftable_writer_new()

Any direct users of `reftable_writer_new()` would simply pass the data directly.

I'll play around and see if this is doable without too much refactoring and have something in the next version.

Show 12 quoted lines

> The latter might be preferable as you basically want to set limits in
> all (most?) situations anyway.
>
>> +	if (w->next)
>> +		BUG("update index modified after writing first block");
>
> Let's not use BUG, but rather return a `REFTABLE_API_ERROR` error. It
> requires a bit more plumbing because we'll also hvae to adapt all
> callers to handle errors. But on the one hand we don't want to die in
> library code. And on the other hand we don't want to keep on adding more
> dependencies on "git-compat-util.h".
>

Fair enough, thanks for explaining.

> Patrick