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

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

[ノウラ | Flare via GitGitGadget]

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

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.

To fix this, we reset slab_alloc, nr, and p to zero/NULL after
freeing the slabs.  This leaves alloc_state in a consistent empty
state for reuse and avoids dangling pointers.

Signed-off-by: Noura EL ALLAM <nouraellm@gmail.com>
---
    Reset slab_alloc and state fields in clear_alloc_state()

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v1
Pull-Request: https://github.com/git/git/pull/2040

 alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/alloc.c b/alloc.c
index 377e80f5dda..6bf9421c123 100644
--- a/alloc.c
+++ b/alloc.c
@@ -49,6 +49,9 @@ void clear_alloc_state(struct alloc_state *s)
 	}
 
 	FREE_AND_NULL(s->slabs);
+	s->slab_alloc = 0;
+	s->nr = 0;
+	s->p = NULL;
 }
 
 static inline void *alloc_node(struct alloc_state *s, size_t node_size)

base-commit: f814da676ae46aac5be0a98b99373a76dee6cedb
-- 
gitgitgadget

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

[Junio C Hamano]

"ノウラ | 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);
 }

[PATCH v2] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare via GitGitGadget]

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
 <nouraellm@gmail.com>

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.

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.

Signed-off-by: ノウラ | Flare <nouraellm@gmail.com>
---
    alloc: fix dangling pointer in alloc_state cleanup

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v2
Pull-Request: https://github.com/git/git/pull/2040

Range-diff vs v1:

 1:  643f14e6225 < -:  ----------- reset slab_alloc and state fields in clear_alloc_state()
 -:  ----------- > 1:  0b980850bdf alloc: fix dangling pointer in alloc_state cleanup


 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 377e80f5dda..c1065551c07 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 3f4a0ad310a..87a47a97095 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 c1553ee4330..986114a6dba 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

Re: [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup

[Torsten Bögershausen]

On Wed, Aug 27, 2025 at 11:28:32PM +0000, ノウラ | Flare via GitGitGadget wrote:
> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
>  <nouraellm@gmail.com>
> 
> 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.
This is good.

> 
> To fix this, this patch:
Style nit, we tend to use the "imperative form" here in Git,
like this:

- Rename allocate_alloc_state() → alloc_state_alloc().
- Replace ...
- Update ...
> 
> - 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.

Re: [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On Wed, Aug 27, 2025 at 11:28:32PM +0000, ノウラ | Flare via GitGitGadget wrote:
>> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
>>  <nouraellm@gmail.com>
>> 
>> 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.
> This is good.
>
>> 
>> To fix this, this patch:
> Style nit, we tend to use the "imperative form" here in Git,
> like this:
>
> - Rename allocate_alloc_state() → alloc_state_alloc().
> - Replace ...
> - Update ...

Thanks.  We also tend to avoid bulleted list.

Re: [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Style nit, we tend to use the "imperative form" here in Git,
>> like this:
>>
>> - Rename allocate_alloc_state() → alloc_state_alloc().
>> - Replace ...
>> - Update ...
>
> Thanks.  We also tend to avoid bulleted list.

Perhaps it needs a bit of clarification.  

If you truly require a bulleted list to enumerate what you did in
the patch, then you may be doing too many things in a single patch.

But in this case, the changes do not even have to be broken down
into bullets.  If you change a called function, it goes without
saying that you have to adjust existing callers.  Perhaps something
like

    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).

should be more in line with what we usually do.

Thanks.

[PATCH v3] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare via GitGitGadget]

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
 <nouraellm@gmail.com>

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@gmail.com>
---
    alloc: fix dangling pointer in alloc_state cleanup
    
    cc: Torsten Bögershausen tboegi@web.de

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@gmail.com>
      


 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

Re: [PATCH v3] alloc: fix dangling pointer in alloc_state cleanup

[Jeff King]

On Fri, Aug 29, 2025 at 01:00:06PM +0000, ノウラ | Flare via GitGitGadget wrote:

> +void alloc_state_free_and_null(struct alloc_state **s_)
>  {
> +	struct alloc_state *s = *s_;
> +
> +	if (!s_ || !*s_) return;
> +

Coverity complains that there's a NULL check here for "s_", but we'll
have already dereferenced it in the initializer for "s".

I don't think any caller passes NULL, so you can't trigger a segfault in
practice. But the code is kind of misleading. Should it just be:

  if (!*s_)
	return;

? Or even just "if (!s)".

-Peff

Re: [PATCH v3] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Aug 29, 2025 at 01:00:06PM +0000, ノウラ | Flare via GitGitGadget wrote:
>
>> +void alloc_state_free_and_null(struct alloc_state **s_)
>>  {
>> +	struct alloc_state *s = *s_;
>> +
>> +	if (!s_ || !*s_) return;
>> +
>
> Coverity complains that there's a NULL check here for "s_", but we'll
> have already dereferenced it in the initializer for "s".
>
> I don't think any caller passes NULL, so you can't trigger a segfault in
> practice. But the code is kind of misleading. Should it just be:
>
>   if (!*s_)
> 	return;
>
> ? Or even just "if (!s)".

Yup, I like that.

The primary point of s_ (parameter with a trailing underscore) is
that we would want to use it as-is as little as possible.  When we
talk about the pointer to alloc_state in this function (not the
location such a pointer is stored at), we should use "s" (not
"*s_").

Thanks for sanity checking.  This may have been my breakage.

[PATCH v4] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare via GitGitGadget]

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
 <nouraellm@gmail.com>

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@gmail.com>
---
    alloc: fix dangling pointer in alloc_state cleanup
    
    cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v4
Pull-Request: https://github.com/git/git/pull/2040

Range-diff vs v3:

 1:  c521b44adb ! 1:  82d228661d alloc: fix dangling pointer in alloc_state cleanup
     @@ alloc.c: 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_;
     ++	struct alloc_state *s;
      +
      +	if (!s_ || !*s_) return;
     ++
     ++	s = *s_;
      +
       	while (s->slab_nr > 0) {
       		s->slab_nr--;


 alloc.c  | 11 +++++++++--
 alloc.h  |  4 ++--
 object.c | 26 ++++++++++----------------
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/alloc.c b/alloc.c
index 377e80f5dd..1483697ca8 100644
--- a/alloc.c
+++ b/alloc.c
@@ -36,19 +36,26 @@ 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;
+
+	if (!s_ || !*s_) return;
+
+	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 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

Re: [PATCH v4] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

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

> +void alloc_state_free_and_null(struct alloc_state **s_)
>  {
> +	struct alloc_state *s;
> +
> +	if (!s_ || !*s_) return;

I still do not see the point of this check.  If the caller passes a
NULL pointer, when they are expected to pass the address of a
pointer variable so that the struct the pointer points at is cleared
and freed, and the pointer variable is NULLed, it is called a
programmer error and they deserve a segfault.  Why would it be
better to sweep such an error under the rug by returning without
anything?  It would delay discovery of such a bug, but for what
gain?

Re: [PATCH v4] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

The point of the check was to avoid subtle crashes
aligning with defensive programming requirements.

If you lean more towards strict contract enforcement
Just say the word.

On 04/09/2025 09:47, Junio C Hamano wrote:
> "ノウラ | Flare via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +void alloc_state_free_and_null(struct alloc_state **s_)
>>   {
>> +	struct alloc_state *s;
>> +
>> +	if (!s_ || !*s_) return;
> I still do not see the point of this check.  If the caller passes a
> NULL pointer, when they are expected to pass the address of a
> pointer variable so that the struct the pointer points at is cleared
> and freed, and the pointer variable is NULLed, it is called a
> programmer error and they deserve a segfault.  Why would it be
> better to sweep such an error under the rug by returning without
> anything?  It would delay discovery of such a bug, but for what
> gain?
>

Re: [PATCH v4] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

ノウラ | Flare <nouraellm@gmail.com> writes:

> The point of the check was to avoid subtle crashes
> aligning with defensive programming requirements.

It is a programming error, period.  Do not silently return.  That's
not being defensive.  That is sweeping a problem under the rug.

We can always do assert() or if (condition) BUG("...") if we are
unsure about the ability of our developers to grok subtle
pre-conditions to call into certain code paths, and I would call
that defensive.

But I do not see a need for _this_ particular call.  Our developers
should be able to see what they are supposed to call it with just
fine.

Thanks.

[PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare via GitGitGadget]

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
 <nouraellm@gmail.com>

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@gmail.com>
---
    alloc: fix dangling pointer in alloc_state cleanup
    
    cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v5
Pull-Request: https://github.com/git/git/pull/2040

Range-diff vs v4:

 1:  82d228661d ! 1:  d04b0258f5 alloc: fix dangling pointer in alloc_state cleanup
     @@ alloc.c: 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;
     -+
     -+	if (!s_ || !*s_) return;
     -+
     -+	s = *s_;
     ++	struct alloc_state *s = *s_;
      +
       	while (s->slab_nr > 0) {
       		s->slab_nr--;


 alloc.c  |  7 +++++--
 alloc.h  |  4 ++--
 object.c | 26 ++++++++++----------------
 3 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/alloc.c b/alloc.c
index 377e80f5dd..3a5d0b2bd8 100644
--- a/alloc.c
+++ b/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 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

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

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

> index 377e80f5dd..3a5d0b2bd8 100644
> --- a/alloc.c
> +++ b/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)

Looking good.

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

Very nice.

Thanks.  Will queue.

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[Jeff King]

On Thu, Sep 04, 2025 at 05:44:16PM +0000, ノウラ | Flare via GitGitGadget wrote:

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

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 but:

  alloc_state_free_and_null(NULL);

is a silly programming error that we do not need to protect against.

-Peff

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> 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 <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

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

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 <peff@peff.net> 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 <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);
>   }

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

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 <peff@peff.net> 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 <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);
>   }

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

By *s I am referring to *s_ so a sanity check with: if (!*s_) return;

On 05/09/2025 02:07, ノウラ | Flare wrote:
> 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 <peff@peff.net> 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 <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);
>>   }

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

 > => s is the double pointer (the address of the caller’s pointer)

For this part as well, it's s_ not s

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[Jeff King]

On Thu, Sep 04, 2025 at 03:26:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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?

Exactly.

> I can by that argument with the reasoning in the updated log message
> below.  Does it good to everybody?

Yep, it looks good to me.

-Peff

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[Jeff King]

On Fri, Sep 05, 2025 at 02:02:39AM +0200, ノウラ | Flare wrote:

> > +    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;

With the correction in your follow-up that this should be:

  if (!*s_) return;

I agree that is the right thing. But it is equivalent to:

  if (!s) return;

since we'll have just assigned "s". Which one to choose is purely a
matter of style. Using "*s_" perhaps makes it more clear that we are
sanity-checking the input (and could happen even before we assign "s").
Using "s" is consistent with the rest of the function in working with
the more direct pointer value. I am happy with either.

> 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

I think this is wrong. "s" is the value we would have allocated from
alloc_state_new(), and what we are actually interested in freeing. We
only see the double-pointer "s_" because we want to modify the caller's
containing pointer.

The FREE_AND_NULL() macro does not itself have this same confusion
because it _is_ a macro. So it is expanded in the caller's context and
does not need the extra layer of indirection.

I do not think this is a good idea, because it obscures things further
for people who are accustomed to C idioms, but one could imagine a more
general macro like:

  #define FREE_AND_NULL_WITH(p, fn) do { fn(p); (p) = NULL; } while (0)
  #define FREE_AND_NULL(p) FREE_AND_NULL_WITH(p, free)

and then you could do:

  FREE_AND_NULL_WITH(o->blob_state, alloc_state_free);

where alloc_state_free() would only receive the single pointer "s", and
would free it but not assign NULL.

-Peff

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

ノウラ | Flare <nouraellm@gmail.com> writes:

> By *s I am referring to *s_ so a sanity check with: if (!*s_) return;

Because we

	s = *s_;

upfront, exactly because we do want the code to segfault if the
caller passes NULL to the function (so s_ that is NULL will cause a
NULL dereference right there), after that happens checking the NULL
ness of s and *s_ is equivalent.

And the whole point of doing "s = *s_" upfront is because readers
can easily get confused when they have to deal with double pointers.
The only reason why we pass the address of the pointer variable is
so that we can assign NULL to it at the very end, and before we can
do so, we want to be able inspect the innards of alloc_state object.
By dereferencing s_ early into s, the code can work with the object
itself without having to worry about following double pointer, so
even though if (!*s_) and if (!s) may be equivalent, writing the
latter is more in line with the whole reason why we have a variable
's' that is separate from 's_'.

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

>>> > I agree that is the right thing. But it is equivalent to:
>>>
>>> > if (!s) return;
>>>
>>> > since we'll have just assigned "s". Which one to choose is purely a
>>> > matter of style. Using "*s_" perhaps makes it more clear that we are
>>> > sanity-checking the input (and could happen even before we assign "s").
>>> > Using "s" is consistent with the rest of the function in working with
>>> > the more direct pointer value. I am happy with either.

I know, I used the original one (**s_) for the explanation. No the check cannot
happen before we assign (that's what I did initially) because tests will
fail given C90 requires declarations to be at the top of the block.

Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare]

On 05/09/2025 16:39, Junio C Hamano wrote:
> ノウラ | Flare <nouraellm@gmail.com> writes:
>
>> By *s I am referring to *s_ so a sanity check with: if (!*s_) return;
> Because we
>
> 	s = *s_;
>
> upfront, exactly because we do want the code to segfault if the
> caller passes NULL to the function (so s_ that is NULL will cause a
> NULL dereference right there), after that happens checking the NULL
> ness of s and *s_ is equivalent.
>
> And the whole point of doing "s = *s_" upfront is because readers
> can easily get confused when they have to deal with double pointers.
> The only reason why we pass the address of the pointer variable is
> so that we can assign NULL to it at the very end, and before we can
> do so, we want to be able inspect the innards of alloc_state object.
> By dereferencing s_ early into s, the code can work with the object
> itself without having to worry about following double pointer, so
> even though if (!*s_) and if (!s) may be equivalent, writing the
> latter is more in line with the whole reason why we have a variable
> 's' that is separate from 's_'.

I see your point, but I previously wrote we could lean more towards

Strict contract enforcement instead of defensive programming

Which would keep if (!s) return; instead of if (!s_ || !*s_) return;

In any case, I believe we all now agree on if (!s) return;

Pushing it right now.

Happy w-e y'all!

[PATCH v6] alloc: fix dangling pointer in alloc_state cleanup

[ノウラ | Flare via GitGitGadget]

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
 <nouraellm@gmail.com>

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: fix dangling pointer in alloc_state cleanup
    
    cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v6
Pull-Request: https://github.com/git/git/pull/2040

Range-diff vs v5:

 1:  d04b0258f5 ! 1:  5afd5b6a26 alloc: fix dangling pointer in alloc_state cleanup
     @@ Commit message
          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
     @@ Commit message
          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 ##
      @@ alloc.c: struct alloc_state {
     @@ alloc.c: struct alloc_state {
      +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--;


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

base-commit: f814da676ae46aac5be0a98b99373a76dee6cedb
-- 
gitgitgadget

Re: [PATCH v6] alloc: fix dangling pointer in alloc_state cleanup

[Junio C Hamano]

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

> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
>  <nouraellm@gmail.com>
>
> 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: fix dangling pointer in alloc_state cleanup
>     
>     cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v6
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v6
> Pull-Request: https://github.com/git/git/pull/2040

Nicely done.
I see no more need for further changes.

> +	if (!s)
> +	    return;

This must be indented with two tabs, i.e.

	if (!s)
		return;

but the copy I have since yesterday already is formatted that way,
so no need to resend this patch.

Thanks.  Let's mark the topic for 'next'.