From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
<nouraellm@xxxxxxxxx>
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.
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@xxxxxxxxx>
---
alloc: fix dangling pointer in alloc_state cleanup
cc: Torsten Bögershausen tboegi@xxxxxx
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v3
Pull-Request: https://github.com/git/git/pull/2040
Range-diff vs v2:
1: 0b980850bd ! 1: c521b44adb alloc: fix dangling pointer in alloc_state cleanup
@@ Metadata
## Commit message ##
alloc: fix dangling pointer in alloc_state cleanup
- 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.
+ 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.
- To fix this, this patch:
-
- - Renames allocate_alloc_state() → alloc_state_alloc().
- - Replaces the “just clear” API with
- alloc_state_free_and_null(struct alloc_state **s_),
- which frees all slabs and the alloc_state itself,
- and then nulls the caller’s pointer.
- - Updates call sites to use the new helpers and drops
- redundant FREE_AND_NULL() calls.
-
- This makes the alloc_state lifecycle API harder to misuse,
- eliminates stale-capacity state,
- and aligns naming with project conventions.
+ 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@xxxxxxxxx>
alloc.c | 9 +++++++--
alloc.h | 4 ++--
object.c | 26 ++++++++++----------------
3 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/alloc.c b/alloc.c
index 377e80f5dd..c1065551c0 100644
--- a/alloc.c
+++ b/alloc.c
@@ -36,19 +36,24 @@ 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_ || !*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);
}
base-commit: f814da676ae46aac5be0a98b99373a76dee6cedb
--
gitgitgadget
