Re: [RFC PATCH v10 32.5/36] unpack_trees: fix memory corruption with split_index when src != dst

From: Elijah Newren <newren@gmail.com>
Date: Apr 23, 2018, 18:05 UTC
Message-ID: <CABPp-BGQy=-k1TN-i2U89x7gJ7z8_ciTM3DJzTnVKsvi94LpBg@mail.gmail.com>
In-Reply-To: <CACsJy8Bmr+rSasRJRbhPUStUjBdVE59M97cGx5_Atii_XSDXaA@mail.gmail.com>

On Mon, Apr 23, 2018 at 10:37 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 17 quoted lines

>>> [1] To me the second parameter should be src_index, not dst_index.
>>> We're copying entries from _source_ index to "result" and we should
>>> also copy extensions from the source index. That line happens to work
>>> only when dst_index is the same as src_index, which is the common use
>>> case so far.
>>
>> That makes sense; this sounds like another fix that should be
>> submitted.  Did you want to submit a patch making that change?  Do you
>> want me to?
>
> I did not look careful enough to make sure it was right and submit a
> patch. But it sounds like it could be another regression if dst_index
> is now not the same as src_index (sorry I didn't look at your whole
> stories and don't if dst_index != src_index is a new thing or not). If
> dst_index is new, moving extensions from that to result index is
> basically no-op, in other words we fail to copy necessary extensions
> over.

Ah, got it, sounds like it should be included in this patch.

A quick summary for you so you don't have to review my whole series:
- All callers of unpack_trees() have dst_index == src_index, until my
  series.
- My series makes merge-recursive.c call unpack_trees() with
  dst_index != src_index (all other callsites unchanged)
- In merge-recursive.c, dst_index points to an entirely new index, so
  yeah we'd be dropping the extensions from the original src_index.

I think all the relevant parts of my series as far as this change is
concerned is the first few diff hunks at
  https://public-inbox.org/git/20180419175823.7946-34-newren@gmail.com/