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

From: Jay Soffian <jaysoffian@gmail.com>
Date: Feb 24, 2009, 06:53 UTC
Message-ID: <76718490902232253o4c7667d9h43f06d81794cef60@mail.gmail.com>
In-Reply-To: <7vfxi4mk8n.fsf@gitster.siamese.dyndns.org>

On Tue, Feb 24, 2009 at 1:17 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> 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.