From: Junio C Hamano <gitster@pobox•com>
To: Atneya Nair <atneya@google•com>
Cc: 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 10:53:02 -0800 [thread overview]
Message-ID: <xmqqsf141pf5.fsf@gitster.g> (raw)
In-Reply-To: <xmqqwmqg38u2.fsf@gitster.g> (Junio C. Hamano's message of "Tue, 05 Mar 2024 09:08:21 -0800")
Junio C Hamano <gitster@pobox•com> writes:
> The use of strintern() comes originally from 3df8fd62 ...
> ..., so they may
> know how safe the change on the config side would be (I still do
> not understand why you'd want to do this in the first place, though,
> especially if you are protecting the callsites with mutex).
The risks of turning code that uses strintern() to use strdup() are
* you will leak the allocated string unless you explicitly free the
string you now own.
* you may consume too much memory if you are creating too many
copies of the same string (e.g. if you need filename for each
line in a file in an application, the memory consumption can
become 1000-fold).
* the code may be taking advantage of the fact that two such
strings can be compared for (in)equality simply by comparing
their addresses, which you would need to adjust to use !strcmp()
and the like.
I just checked to make sure that the last one is not the case for
our codebase, and you said you didn't see the second one is the case,
so the change may be a safe one to make.
One more thing. Do not use strdup() without checking its return
value for failure. It would be an easy fix to use xstrdup() instead.
Thanks.
>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..d7f73d8745 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
>> enum config_scope scope,
>> struct key_value_info *out)
>> {
>> - out->filename = strintern(cs->name);
>> + out->filename = strdup(cs->name);
>> out->origin_type = cs->origin_type;
>> out->linenr = cs->linenr;
>> out->scope = scope;
>> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>>
>> strbuf_release(&top->value);
>> strbuf_release(&top->var);
>> + free(kvi.filename);
>>
>> return ret;
>> }
next prev parent reply other threads:[~2024-03-05 18:53 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 [this message]
2024-03-06 1:23 ` Jeff King
2024-03-06 1:58 ` Junio C Hamano
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=xmqqsf141pf5.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=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