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

From: Junio C Hamano <[email-masked]>
Date: Wed, 05 Jun 2024 23:02:07 -0700

Patrick Steinhardt <[email-masked]> writes:

> The buffers of cached objects are never modified, but are still stored
> as a non-constant pointer. This will cause a compiler warning once we
> enable the `-Wwrite-strings` compiler warning as we assign an empty
> constant string when initializing the static `empty_tree` cached object.
>
> Convert the field to be constant. This requires us to shuffle around
> the code a bit because we memcpy(3P) into the allocated buffer in
> `pretend_object_file()`. This is easily fixed though by allocating the
> buffer into a temporary variable first.
>
> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  object-file.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

I haven't dug into the exact details, but I noticed that t6130,
t7010, and t8002 started breaking linux-leaks/linux-reftable-leaks
CI jobs for 'seen'.  'seen' excluding ps/no-writable-strings seems
to pass the tests, and bisection indicates that 'seen' excluding
ps/no-writable-strings after this step [12/27] still passes, but
once this step is included, the leak tests break.

> diff --git a/object-file.c b/object-file.c
> index 610b1f465c..3afe9fce06 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -277,7 +277,7 @@ int hash_algo_by_length(int len)
>  static struct cached_object {
>  	struct object_id oid;
>  	enum object_type type;
> -	void *buf;
> +	const void *buf;
>  	unsigned long size;
>  } *cached_objects;
>  static int cached_object_nr, cached_object_alloc;
> @@ -1778,6 +1778,10 @@ 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);
>  
>  	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) ||

There is an early return around here.  Perhaps we are leaking co_buf
when we hit it?

> @@ -1787,8 +1791,7 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,
>  	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;
>  	oidcpy(&co->oid, oid);
>  	return 0;
>  }