Re: [PATCH] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Jay Soffian <jaysoffian@gmail.com>]

On Tue, Feb 24, 2009 at 1:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> This issue has gone undetected as the existing callers of match_heads()
>> call it only once and then terminate, w/o ever bothering to free the src
>> or dst lists.
>
> Thanks.
>
> It sounds more like existing function and usage was alright but your new
> caller made it into an issue.

Well, maybe. The existing function alters the passed in dst argument
(a ref list) in two ways:

1) It potentially adds new refs to the tail of the list.
2) For the existing and new refs, it populates their peer_ref field.
When it does this, it sometimes just points it at an existing ref in
the src list, but sometimes it allocates a new ref and points to that.

So there is no sane way to free the result. As there is no indication
that the caller shouldn't be able to free the src and dst lists after
it is done, it was IMO a latent bug.

Initially I tried to fix this by modifying free_refs() to make two
passes over the list passed to it, once to mark each peer_ref, then a
second time to clear only those peer_refs it had not already seen. I
did this because when I first saw the double free I was only freeing
the dst list. It took me a while to realize the cause was two refs in
the dst list having the same peer_ref.

But then I realized my caller needed to clear the src list as well,
and saw that the problem was really in match_heads(), not in free'ing
the result.

> So I'd queue this as the first patch in the 13-patch series, dropping 10/13?
> What should happen to 11/13?

I sent you a message off-list you may not have read yet. I'm
re-rolling the entire js/remote-set-head for you off of the current
master to get rid of any dependencies on pu, with jk/head-lookup on
the end of the series instead of in the middle. I needed to rebase off
master since I have a dependency on cfa1ee6 which is now in master,
but wasn't when you spawned js/remote-set-head initially.

j.