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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2024, 18:53 UTC
Message-ID: <xmqqsf141pf5.fsf@gitster.g>
In-Reply-To: <xmqqwmqg38u2.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

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

Show 21 quoted lines

>> 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;
>>  }