Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 4, 2025, 22:26 UTC
- Message-ID
- <xmqqjz2d7t2q.fsf@gitster.g>
- In-Reply-To
- <20250904204932.GD30633@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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); }
-- 2.51.0-314-g9a15cfd6dc