From: ノウラ | Flare Date: Fri, 05 Sep 2025 00:07:10 GMT Subject: Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup Message-ID: <52fb3fea-4842-4532-8572-df41537da5f9@gmail.com> In-Reply-To: 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); > }