Subject: Re: [PATCH 2/2] object-file: free(*contents) only in read_loose_object() caller

From: Junio C Hamano <[email-masked]>
Date: Thu, 11 Nov 2021 10:54:18 -0800

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index d87c28a1cc4..27b9e78094d 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -605,7 +605,7 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
>  	struct object *obj;
>  	enum object_type type = OBJ_NONE;
>  	unsigned long size;
> -	void *contents;
> +	void *contents = NULL;
>  	int eaten;
>  	struct object_info oi = OBJECT_INFO_INIT;
>  	struct object_id real_oid = *null_oid();
> @@ -630,6 +630,7 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
>  			    path);
>  	if (err < 0) {
>  		errors_found |= ERROR_OBJECT;
> +		free(contents);
>  		return 0; /* keep checking other objects */
>  	}
>  
> diff --git a/object-file.c b/object-file.c
> index ac476653a06..c3d866a287e 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2528,8 +2528,6 @@ int read_loose_object(const char *path,
>  	char hdr[MAX_HEADER_LEN];
>  	unsigned long *size = oi->sizep;
>  
> -	*contents = NULL;
> -
>  	map = map_loose_object_1(the_repository, path, NULL, &mapsize);
>  	if (!map) {
>  		error_errno(_("unable to mmap %s"), path);
> @@ -2559,10 +2557,9 @@ int read_loose_object(const char *path,
>  			goto out;
>  		}
>  		if (check_object_signature(the_repository, expected_oid,
> -					   *contents, *size, oi->type_name->buf, real_oid)) {
> -			free(*contents);
> +					   *contents, *size,
> +					   oi->type_name->buf, real_oid))
>  			goto out;
> -		}
>  	}

Yeah, I have to say that read_loose_object() that frees *contents
without clearing *contents to NULL only because it wants to signal
if the failure comes from check_object_signature() step is quite
ugly.  Making the caller responsible for freeing (in other words,
when caller's *contents is non-NULL after function returns, it
always has a valid piece of memory to be freed) makes the contract
easier to explain.