Re: [PATCH] reset slab_alloc and state fields in clear_alloc_state()
From: Junio C Hamano <hidden>
Date: 2025-08-27 02:07:22
"ノウラ | Flare via GitGitGadget" [off-list ref] writes:
From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9?= <redacted>
This name used on the in-body From: line must match who signs off the patch below on Signed-off-by: line.
clear_alloc_state() freed all slabs and nulled the slabs pointer but left slab_alloc, nr, and p unchanged. If the alloc_state is reused, ALLOC_GROW() can wrongly assume that the slab array is already allocated because slab_alloc still holds a stale nonzero capacity. In that case s->slabs remains NULL and the next dereference writes through a NULL pointer, causing undefined behavior.
Let's not give such a misuse-prone API to consumers, then. How about doing something like this instead? As Documentation/CodingGuidelines says the API functions for subsystem S should in general be called S_<something>, let's rename allocate_alloc_state() to alloc_state_alloc(), get rid of the "just clear" function and make it alloc_state_free_and_null() to do just that. alloc.c | 7 +++++-- alloc.h | 4 ++-- object.c | 26 +++++++++++--------------- 3 files changed, 18 insertions(+), 19 deletions(-)
diff --git c/alloc.c w/alloc.c
index 377e80f5dd..3a5d0b2bd8 100644
--- c/alloc.c
+++ w/alloc.c@@ -36,19 +36,22 @@ 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_; + 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 c/alloc.h w/alloc.h
index 3f4a0ad310..cd6ed16ffb 100644
--- c/alloc.h
+++ w/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 c/object.c w/object.c
index c1553ee433..4469755ea6 100644
--- c/object.c
+++ w/object.c@@ -517,11 +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 +573,12 @@ 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); }