Re: [PATCH] reset slab_alloc and state fields in clear_alloc_state()

[Junio C Hamano <gitster@pobox.com>]

"ノウラ | Flare via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9?= <nea@odoo.com>

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);
 }