From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Atneya Nair <atneya@google•com>,
git@vger•kernel.org, jeffhost@microsoft•com, me@ttaylorr•com,
nasamuffin@google•com, Tanay Abhra <tanayabh@gmail•com>,
Glen Choo <glencbz@gmail•com>
Subject: Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
Date: Tue, 05 Mar 2024 17:58:09 -0800 [thread overview]
Message-ID: <xmqqzfvcunny.fsf@gitster.g> (raw)
In-Reply-To: <20240306012323.GA3817803@coredump.intra.peff.net> (Jeff King's message of "Tue, 5 Mar 2024 20:23:23 -0500")
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.
next prev parent reply other threads:[~2024-03-06 1:58 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-05 1:21 [RFC PATCH 0/3] Parallel submodule status Atneya Nair
2024-03-05 1:21 ` [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe Atneya Nair
2024-03-05 2:22 ` Junio C Hamano
2024-03-05 4:29 ` Eric Sunshine
2024-03-12 20:38 ` Atneya Nair
2024-03-06 8:13 ` Jean-Noël Avila
2024-03-06 16:57 ` Junio C Hamano
2024-03-12 20:44 ` Atneya Nair
2024-03-05 1:21 ` [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe Atneya Nair
2024-03-05 17:08 ` Junio C Hamano
2024-03-05 18:53 ` Junio C Hamano
2024-03-06 1:23 ` Jeff King
2024-03-06 1:58 ` Junio C Hamano [this message]
2024-03-12 22:13 ` Atneya Nair
2024-03-12 22:15 ` Atneya Nair
2024-03-13 17:51 ` Junio C Hamano
2024-03-05 1:21 ` [RFC PATCH 3/3] Preload submodule state in refresh_index Atneya Nair
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzfvcunny.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=atneya@google$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=glencbz@gmail$(echo .)com \
--cc=jeffhost@microsoft$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=nasamuffin@google$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=tanayabh@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox