Re: [PATCH v5 12/27] object-file: mark cached object buffers as const

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2024, 17:54 UTC
Message-ID: <xmqqfrtqlzye.fsf@gitster.g>
In-Reply-To: <5cd014c22cb2fc7e34666aa6bd036d3cc4ce9039.1717667854.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 8 quoted lines

> @@ -1778,17 +1778,22 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,
>  			struct object_id *oid)
>  {
>  	struct cached_object *co;
> +	char *co_buf;
> +
> +	co_buf = xmalloc(len);
> +	memcpy(co_buf, buf, len);

I do not see why we need to do this so early. The copy is not used or buf gets modified by the call to hash_object_file(), so ...

Show 14 quoted lines

>  	hash_object_file(the_hash_algo, buf, len, type, oid);
>  	if (repo_has_object_file_with_flags(the_repository, oid, OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT) ||
> -	    find_cached_object(oid))
> +	    find_cached_object(oid)) {
> +		free(co_buf);
>  		return 0;
> +	}
>  	ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);
>  	co = &cached_objects[cached_object_nr++];
>  	co->size = len;
>  	co->type = type;
> -	co->buf = xmalloc(len);
> -	memcpy(co->buf, buf, len);
> +	co->buf = co_buf;

... wouldn't this be a better place to do the "copy to the heap memory pointed by a writable pointer and then point that piece of memory with a read-only pointer" pattern?

>  	oidcpy(&co->oid, oid);
>  	return 0;
>  }