Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe

[Junio C Hamano <gitster@pobox.com>]

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.

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