Patrick Steinhardt <ps@xxxxxx> writes:
> On Mon, Jun 02, 2025 at 11:57:24AM +0200, Karthik Nayak wrote:
>> The commit 23fc8e4f61 (refs: implement batch reference update support,
>> 2025-04-08) introduced support for batched reference updates. This
>> allows users to batch updates together, while allowing some of the
>> updates to fail.
>>
>> Under the hood, batched updates use the reference transaction mechanism.
>> Each update which fails is marked as such. Any failed updates must be
>> skipped over in the rest of the code, as they wouldn't apply any more.
>> In two of the loops within 'files_transaction_finish()' of the files
>> backend, the failed updates aren't skipped over. This can cause a
>> SEGFAULT otherwise. Add the missing skips and a test to validate the
>> same.
>
> Curious -- we do have tests, so why don't any of them hit this issue?
>
We don't hit this edge case in any of our tests I presume. This
basically requires a deletion request with an expected old OID, however
the reference should be non-existent.
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> refs/files-backend.c | 7 +++++++
>> t/t1400-update-ref.sh | 14 ++++++++++++++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 4d1f65a57a..c4a0f29072 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
>> */
>> for (i = 0; i < transaction->nr; i++) {
>> struct ref_update *update = transaction->updates[i];
>> +
>> + if (update->rejection_err)
>> + continue;
>> +
>> if (update->flags & REF_DELETING &&
>> !(update->flags & REF_LOG_ONLY) &&
>> !(update->flags & REF_IS_PRUNING)) {
>
> Ok. And the reftable backend doesn't need the same treatment? Probably
> doesn't because we queue all updates via `queue_transaction_update()`,
> which knows to not add them to the list of updates in case any error has
> happened. And in `_finish()` we iterate through that list of queued
> updates instead of the global list of updates.
>
Exactly, all writes in the reftable backend are propagated finally via
'write_transaction_table()', which checks for rejections. The single
point of entry acts as a good gatekeeper.
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index d29d23cb89..e9a605d0ba 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -2299,6 +2299,20 @@ do
>> test_grep -q "refname conflict" stdout
>> )
>> '
>> +
>> + test_expect_success "stdin $type batch-updates delete non-existent ref" '
>> + git init repo &&
>> + test_when_finished "rm -fr repo" &&
>> + (
>> + cd repo &&
>> + test_commit commit &&
>> + head=$(git rev-parse HEAD) &&
>> +
>> + format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
>> + git update-ref $type --stdin --batch-updates <stdin >stdout &&
>> + test_grep -q "reference does not exist" stdout
>
> We typically don't silence the output of `test_grep`.
>
> Patrick
Will change, Thanks!
Attachment:
signature.asc
Description: PGP signature
