From: ノウラ | Flare Date: Fri, 05 Sep 2025 00:25:55 GMT Subject: Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup Message-ID: <88ae6c7b-0a69-48bd-9e73-672331a22197@gmail.com> In-Reply-To: <52fb3fea-4842-4532-8572-df41537da5f9@gmail.com> By *s I am referring to *s_ so a sanity check with: if (!*s_) return; On 05/09/2025 02:07, ノウラ | Flare wrote: > No. I am confused here. > > > It is a programming error, period.  Do not silently return. That's > > not being defensive.  That is sweeping a problem under the rug. > > Yet > > > +    if (!s) return; > > However, I agree with Peff. After calling alloc_state_free_and_null(&foo) > Having foo == NULL is an expected behavior, especially since the function > Is designed to free the memory and null out the caller’s pointer using > A double pointer ensuring the helper is idempotent > > So, calling it again on the same pointer is safe because it simply > no-ops if the memory is already freed > > Regarding the sanity check, it should be: > > + if (!*s) return; > > The reasoning is the following: > > => s is the double pointer (the address of the caller’s pointer) > => *s is the actual pointer to the memory we want to free > > Thus, we check !*s to allow the function to safely handle already-NULL > pointers > But if we instead checked !s then we would be testing whether the > caller passed > A NULL double pointer which is a programmer error > So silently returning on !s would hide a bug > > Finally, I'd appreciate more explicit instructions. > > On 05/09/2025 00:26, Junio C Hamano wrote: >> Jeff King 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 >> 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 >> Helped-by: Jeff King >> Signed-off-by: Junio C Hamano >> --- >>   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); >>   }