Re: Infinite loop + memory leak in annotate_refs_with_symref_info

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 11, 2023, 20:25 UTC
Message-ID: <ZDXCKecwxo36fALm@nand.local>
In-Reply-To: <39035D34-8548-44B0-BBBB-5C36B3876C4A@lophus.org>

Hi Jonas,

On Tue, Apr 11, 2023 at 10:53:59PM +0300, Jonas Haag wrote:

Show 6 quoted lines

> Hello!
>
> There is an infinite loop with an accompanying memory leak in
> annotate_refs_with_symref_info that was introduced in Git 2.28 (I
> think in commit 2c6a403: “connect: add function to parse multiple v1
> capability values”).

I'm not familiar with Klaus and don't have it installed, but a couple of questions: were you able to reproduce this result with any other forges or tools, and were you able to confirm that 2c6a403 is the culprit via a bisection?

In case the answer to the latter question is "yes", I cc'd brian carlson on this thread, since they are the original author of that patch.

Show 8 quoted lines

> I believe the bug in Git is in connect.c, function
> parse_feature_value, in the updating of `*offset`: It doesn’t seem to
> take into account that `feature_list` has already been offset by
> `*offset`. I believe the update needs to use `*offset +=` instead of
> `*offset =`. When I make this change, the infinite loop seems to go
> away, and cloning via Klaus/Dulwich will fail with “invalid index-pack
> output”. Cloning from github.com works, although I’m not sure if
> that’s a relevant smoke test in this case.

I'm not sure I understand. Looking at the relevant bits in connect.c::parse_feature_value(), it all seems correct to me, since the beginning of `feature_list` is adjusted by the current value of `*offset`.

> [1] https://github.com/jonashaag/klaus

Thanks, Taylor