On Mon, Jun 13, 2016 at 08:14:43PM -0400, Keith McGuigan wrote:
> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html