Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2022, 17:23 UTC
Message-ID: <xmqqy1y9905h.fsf@gitster.g>
In-Reply-To: <9195fe51-a696-1fb7-3617-a5b65358a806@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 14 quoted lines

>> diff --git a/read-cache.c b/read-cache.c
>> index 96ce489c7c5..e61af3a3d4d 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>>  	ce->ce_namelen = len;
>>  	ce->index = 0;
>>  	oidread(&ce->oid, ondisk->data);
>> -	memcpy(ce->name, name, len);
>> -	ce->name[len] = '\0';
>
> This removal looks  correct to me.  The extra copying was added by
> 575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
> but its commit message doesn't mention it.

I agree with the analysis. When we are prefix-compressing the entry, name[] may be much shorter than len, so this memcpy() may potentially be reading beyond the end of the on-disk buffer, so the original code is not just redundant, but it may simply be wrong.

Thanks, both.  Will queue.

Show 18 quoted lines

>>
>>  	if (expand_name_field) {
>>  		if (copy_len)
>
> Some more context lines would help to see the redundancy; here they are:
>
> 	if (expand_name_field) {
> 		if (copy_len)
> 			memcpy(ce->name, previous_ce->name, copy_len);
> 		memcpy(ce->name + copy_len, name, len + 1 - copy_len);
> 		*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
> 	} else {
> 		memcpy(ce->name, name, len + 1);
> 		*ent_size = ondisk_ce_size(ce);
> 	}
>
>>
>> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085