Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3296 bytes --]

On Mon, Mar 04, 2024 at 08:28:17AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> > Now there is a very particular edge case in this situation: when
> >> > preparing an empty ref transacton, we end up returning whatever value
> >> > `read_ref_without_reload()` returned to the caller. Under normal
> >> > conditions this would be fine: "HEAD" should usually exist, and thus the
> >> > function would return `0`. But if "HEAD" doesn't exist, the function
> >> > returns a positive value which we end up returning to the caller.
> >> >
> >> > Fix this bug by resetting the return code to `0` and add a test.
> 
> So this _will_ surface as a bug when the other change in the series
> is applied, but it nevertheless is worth fixing independently of the
> other one, because ...
> 
> >> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
> >> >  				      &head_referent, &head_type);
> >> >  	if (ret < 0)
> >> >  		goto done;
> >> > +	ret = 0;
> 
> ... after "if the refs->err records an error already, skip
> everything we do and return to the caller", we should take the
> ownership of what we return (which will be in "ret") from now on.
> 
> So the current code uses "ret" as an uninitialized variable, even
> not technically so because it is "initialized" to "refs->err"
> upfront, and this is like a fix of uninitialized variable use.

The problem is a bit different. We call `read_ref_without_reload()` to
look up the "HEAD" ref, which will return a positive value in case the
ref wasn't found. This is customary in the reftable library: positive
error values indicate that an iter is over, and thus by extension that a
value was not found. It's fine though if the ref doesn't exist, and we
handle that case gracefully.

The only exception is when the transaction is also empty. In that case,
we skip the loop and thus end up not assigning to `ret` anymore. Thus,
the positive error code we still have in `ret` from the failed "HEAD"
lookup gets returned to the caller, which is wrong.

So it's not uninitialized, it rather is stale.

But yes, the bug _can_ be hit independently of the second patch in this
series. It's just really unlikely as a repo without "HEAD" is considered
to be broken anyway.

> >> So this is not really a problem in this function, it's more of that
> >> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.
> >
> > Well, yes. I'd claim that it is a problem in this function because it
> > returns positive even though the transaction was prepared successfully.
> >
> >> Nit: would be nice to have a comment about why overriding this value is
> >> ok.
> >
> > True.
> 
> Yup.  It seems we will see a v2 for updating the test code as well,
> so I'll assume that you'd explain this as an independent fix (as
> well as a required preliminary fix for the other one).

I see that the patch series has been merged to "next" a few days ago
though, and is slated to be merged to "master". That's why I refrained
from sending a v2.

I can send a follow-up patch to remove the useless variable assignment
in the test, but other than that I don't think anything needs to change
here. Or did I miss anything else?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]