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

Jeff King [off-list ref] 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.