read-cache.c: reduce unnecessary cache entry name copying

[adlternative]

Index cache entries name are copied twice wrongly, so reduce the first one
to fix it.

cc: Junio C Hamano gitster@pobox.com
cc: brian m. carlson sandals@crustytoothpaste.net
cc: Christian Couder christian.couder@gmail.com
cc: René Scharfe l.s.r@web.de

[adlternative]

/submit

[gitgitgadget]

Submitted as pull.1249.git.1654436248249.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1249/adlternative/zh/rm-ce-copy-v1

To fetch this version to local tag pr-1249/adlternative/zh/rm-ce-copy-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1249/adlternative/zh/rm-ce-copy-v1

[gitgitgadget]

On the Git mailing list, René Scharfe wrote (reply to this):

Am 05.06.22 um 15:37 schrieb ZheNing Hu via GitGitGadget:
> From: ZheNing Hu <adlternative@gmail.com>
>
> In function create_from_disk, we have already copy the cache
> entries name from disk or previous cache entry, we can reduce
> unnecessary copy before that.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     read-cache.c: reduce unnecessary cache entry name copying
>
>     Index cache entries name are copied twice wrongly, so reduce the first
>     one to fix it.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1249
>
>  read-cache.c | 2 --
>  1 file changed, 2 deletions(-)
>
> 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.

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

[gitgitgadget]

User René Scharfe <l.s.r@web.de> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

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

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

[gitgitgadget]

This branch is now known as zh/read-cache-copy-name-entry-fix.

[gitgitgadget]

This patch series was integrated into seen via git@2d0146e.

[gitgitgadget]

This patch series was integrated into seen via git@ef2aa39.

[gitgitgadget]

This patch series was integrated into seen via git@9f8b8b0.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch zh/read-cache-copy-name-entry-fix on the Git mailing list:

Remove redundant copying (with index v3 and older) or possible
over-reading beyond end of mmapped memory (with index v4) has been
corrected.

Will merge to 'next'.
source: <pull.1249.git.1654436248249.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@37a506d.

[gitgitgadget]

This patch series was integrated into next via git@760f43d.

[gitgitgadget]

This patch series was integrated into seen via git@3140c98.

[gitgitgadget]

This patch series was integrated into seen via git@4956152.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch zh/read-cache-copy-name-entry-fix on the Git mailing list:

Remove redundant copying (with index v3 and older) or possible
over-reading beyond end of mmapped memory (with index v4) has been
corrected.

Will merge to 'master'.
source: <pull.1249.git.1654436248249.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@d007935.

[gitgitgadget]

This patch series was integrated into seen via git@113656e.

[gitgitgadget]

This patch series was integrated into master via git@113656e.

[gitgitgadget]

This patch series was integrated into next via git@113656e.

[gitgitgadget]

Closed via 113656e.