Re: [PATCH 3/3] index-pack: allow revisiting REF_DELTA chains

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 25, 2025, 03:49 UTC
Message-ID: <d7313975-24a7-48eb-9626-66a3e53d4234@gmail.com>
In-Reply-To: <xmqqjz79texy.fsf@gitster.g>

On 4/24/2025 5:41 PM, Junio C Hamano wrote:

Show 25 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> As detailed in the previous changes to t5309-pack-delta-cycles.sh, the
>> logic within 'git index-pack' to analyze an incoming thin packfile with
>> REF_DELTAs is suspect. The algorithm is overly cautious around delta
>> cycles, and that leads in fact to failing even when there is no cycle.
>>
>> This change adjusts the algorithm to no longer fail in these cases. In
>> fact, these cycle cases will no longer fail but more importantly the
>> valid cases will no longer fail, either. The resulting packfile from the
>> --fix-thin operation will not have cycles either since REF_DELTAs are
>> forbidden from the on-disk format and OFS_DELTAs are impossible to write
>> as a cycle.
> 
> Loosening cycle avoidance indeed is worrysome.  How do we guarantee
> "since REF_DELTAs are forbidden from the on-disk format" (it is
> obvious with OFS_DELTA only you cannot form a cycle)?  By code
> inspection and in-code comment for the code path of "--fix-thin"?
> 
> I think the most interesting question would be how, with the
> loosening of the cycle check in this patch, we would still protect
> against a malicious on-the-wire packdata that has a cycle in it
> already, where deltas cannot be resolved.

The good news is two-fold:

1. We have a test in t5309 (index-pack detects REF_DELTA cycles) that
   checks this case and did not change behavior with this patch.

2. If we don't already have any of the objects that exist in the cycle,
   then we can't _start_ expanding the objects to their full contents
   as we don't have their bases. So what is left is a list of REF_DELTAs
   that have not been processed but also a list of base object IDs that
   we can't load. We'll fail without writing those objects to disk.

Does that satisfy your concerns in this space?

Thanks, -Stolee