Re: [PATCH 1/1] Don't free remote->name after fetch

From: Jeff King <peff@peff.net>
Date: Jun 14, 2016, 07:52 UTC
Message-ID: <20160614075252.GA12563@sigill.intra.peff.net>
In-Reply-To: <CALnYDJNS9QAtu37a76Q6N3C=GRbfgU8Xq3g7F1q7vX+b=rwOOA@mail.gmail.com>

On Mon, Jun 13, 2016 at 08:14:43PM -0400, Keith McGuigan wrote:

Show 9 quoted lines

> Right.  The string_list ends up getting (potentially) populated with a
> mix of dup'd and borrowed values.  I figured it was safer to leak here
> (especially as we're on the way out anyway), than free memory that
> shouldn't be freed.
> 
> Actually, what motivates this (and I apologize that I didn't say this
> earlier) is that we added (in our repo) a bit of stats collection code
> that executes after the string_list_clear(), and calls remote_get()
> which goes all sideways when some of its memory has been freed.

Yeah, I think nobody noticed because we don't have any actual code that runs after this string_list_clear(), but that doesn't make it not-buggy.

Show 5 quoted lines

> As an alternative, I could xstrdup each instance where remote->name is
> appended, which would make the string_list a homogenous dup'd list,
> which we could then free.  If you prefer that I'll do a re-roll in
> that style (it just seemed to me at the time like it would be doing
> some useless allocations).  I don't much mind either way.

That sounds much better. Fixing any case like this is really a two-part thing:

  1. Making the memory ownership policy of the string_list consistent
     (either all allocated, or all not). And if you have _some_ items
     which must be newly allocated (i.e., there is no other place to own
     them), then the only consistent solution is to allocate all of
     them.

  2. Matching the strdup_strings field to that policy.

     The main() function should not have to play tricks with setting
     list->strdup_strings after the fact. It should set it up front,
     and the functions it calls should use string_list_append as
     appropriate.

-Peff