Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
Jeff King <peff@peff.net> writes:
> There is one more, I think: if you _do_ free the allocated string to
> avoid the leak you mention, then some other code which was relying on
> the lifetime of that string to be effectively infinite will now have a
> user-after-free.
Ah, yes, you're right. I completely forgot about that shallow copy.
Show 17 quoted lines
> A few other things to note, looking at this code:
>
> - isn't kvi->path in the same boat? We do not duplicate it at all, so
> it seems like the shallow copy made in the configset could cause a
> user-after-free.
>
> - the "fix" I showed above hits your point 2: now we are making a lot
> more copies of that string. I will note that we're already making a
> lot of copies of the kvi struct in the first place, so unless you
> have really long pathnames, it probably isn't a big difference.
>
> But it possibly could make sense to have the configset own a single
> duplicate string, and then let the kvi structs it holds point to
> that string. But IMHO all of this should be details of the configset
> code, and the main config-iteration code should not have to worry
> about this at all. I.e., I think kvi_from_source() should not be
> duplicating anything in the first place.
Thanks for a detailed write-up.
Junio C Hamano <gitster@pobox.com>