Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

> Jeff King <peff@peff.net> writes:
>
>> It's probably not worth going back and forth on this too much, but I
>> thought the happy medium was:
>>
>>    if (!s)
>> 	return;
>>
>> That is, it is perfectly reasonable and friendly for it to be a noop to
>> free-and-null a NULL value (either never initialized, or already freed).
>> The overkill was worrying about whether somebody passed in a NULL
>> double-pointer. I.e., doing:
>>
>>    alloc_state_free_and_null(&foo);
>>
>> is reasonable and should be idempotent
> ... when foo == NULL, e.g., after alloc_state_free_and_null(&foo)
> has just successfully returned?
>
> I can by that argument with the reasoning in the updated log message
> below.  Does it good to everybody?
>
> Thanks.
>
> --- >8 ---
> From: ノウラ | Flare <nouraellm@gmail.com>
> Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup
>
> All callers of clear_alloc_state() immediately free what they
> cleared, so currently it does not hurt anybody that the
> alloc_state is left in an unreusable state, but it is an
> error-prone API. Replace it with a new function that clears but
> in addition frees the structure, as well as NULLing the pointer
> that points at it and adjust existing callers.
>
> As it is a moral equivalent of FREE_AND_NULL(), except that what it
> frees has internal structure that needs to be cleaned, allow the
> helper to be called twice in a row, by making a call with a pointer
> to a pointer variable that already is NULLed.
>
> While at it, rename allocate_alloc_state() and name the new
> function alloc_state_free_and_null(), to follow more closely the
> function naming convention specified in the CodingGuidelines
> (namely, functions about S are named with S_ prefix and then
> verb).
>
> Signed-off-by: ノウラ | Flare <nouraellm@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   alloc.c  | 10 ++++++++--
>   alloc.h  |  4 ++--
>   object.c | 26 ++++++++++----------------
>   3 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/alloc.c b/alloc.c
> index 377e80f5dd..533a045c2a 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -36,19 +36,25 @@ struct alloc_state {
>   	int slab_nr, slab_alloc;
>   };
>   
> -struct alloc_state *allocate_alloc_state(void)
> +struct alloc_state *alloc_state_alloc(void)
>   {
>   	return xcalloc(1, sizeof(struct alloc_state));
>   }
>   
> -void clear_alloc_state(struct alloc_state *s)
> +void alloc_state_free_and_null(struct alloc_state **s_)
>   {
> +	struct alloc_state *s = *s_;
> +
> +	if (!s)
> +		return;
> +
>   	while (s->slab_nr > 0) {
>   		s->slab_nr--;
>   		free(s->slabs[s->slab_nr]);
>   	}
>   
>   	FREE_AND_NULL(s->slabs);
> +	FREE_AND_NULL(*s_);
>   }
>   
>   static inline void *alloc_node(struct alloc_state *s, size_t node_size)
> diff --git a/alloc.h b/alloc.h
> index 3f4a0ad310..87a47a9709 100644
> --- a/alloc.h
> +++ b/alloc.h
> @@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);
>   void *alloc_tag_node(struct repository *r);
>   void *alloc_object_node(struct repository *r);
>   
> -struct alloc_state *allocate_alloc_state(void);
> -void clear_alloc_state(struct alloc_state *s);
> +struct alloc_state *alloc_state_alloc(void);
> +void alloc_state_free_and_null(struct alloc_state **s_);
>   
>   #endif
> diff --git a/object.c b/object.c
> index c1553ee433..986114a6db 100644
> --- a/object.c
> +++ b/object.c
> @@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
>   	memset(o, 0, sizeof(*o));
>   
>   	o->repo = repo;
> -	o->blob_state = allocate_alloc_state();
> -	o->tree_state = allocate_alloc_state();
> -	o->commit_state = allocate_alloc_state();
> -	o->tag_state = allocate_alloc_state();
> -	o->object_state = allocate_alloc_state();
> -
> +	o->blob_state = alloc_state_alloc();
> +	o->tree_state = alloc_state_alloc();
> +	o->commit_state = alloc_state_alloc();
> +	o->tag_state = alloc_state_alloc();
> +	o->object_state = alloc_state_alloc();
>   	o->is_shallow = -1;
>   	CALLOC_ARRAY(o->shallow_stat, 1);
>   
> @@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)
>   	o->buffer_slab = NULL;
>   
>   	parsed_object_pool_reset_commit_grafts(o);
> -	clear_alloc_state(o->blob_state);
> -	clear_alloc_state(o->tree_state);
> -	clear_alloc_state(o->commit_state);
> -	clear_alloc_state(o->tag_state);
> -	clear_alloc_state(o->object_state);
> +	alloc_state_free_and_null(&o->blob_state);
> +	alloc_state_free_and_null(&o->tree_state);
> +	alloc_state_free_and_null(&o->commit_state);
> +	alloc_state_free_and_null(&o->tag_state);
> +	alloc_state_free_and_null(&o->object_state);
>   	stat_validity_clear(o->shallow_stat);
> -	FREE_AND_NULL(o->blob_state);
> -	FREE_AND_NULL(o->tree_state);
> -	FREE_AND_NULL(o->commit_state);
> -	FREE_AND_NULL(o->tag_state);
> -	FREE_AND_NULL(o->object_state);
>   	FREE_AND_NULL(o->shallow_stat);
>   }