From: "ノウラ | Flare" <nouraellm@gmail•com>
To: Junio C Hamano <gitster@pobox•com>, Jeff King <peff@peff•net>
Cc: "ノウラ | Flare via GitGitGadget" <gitgitgadget@gmail•com>,
git@vger•kernel.org
Subject: Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup
Date: Fri, 5 Sep 2025 02:02:39 +0200 [thread overview]
Message-ID: <8f831259-7372-4357-b059-cc21f7a04864@gmail.com> (raw)
In-Reply-To: <xmqqjz2d7t2q.fsf@gitster.g>
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);
> }
next prev parent reply other threads:[~2025-09-05 0:02 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-26 19:57 [PATCH] reset slab_alloc and state fields in clear_alloc_state() ノウラ | Flare via GitGitGadget
2025-08-27 2:07 ` Junio C Hamano
2025-08-27 23:28 ` [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup ノウラ | Flare via GitGitGadget
2025-08-28 19:29 ` Torsten Bögershausen
2025-08-28 19:47 ` Junio C Hamano
2025-08-28 20:01 ` Junio C Hamano
2025-08-29 13:00 ` [PATCH v3] " ノウラ | Flare via GitGitGadget
2025-09-03 11:18 ` Jeff King
2025-09-03 21:59 ` Junio C Hamano
2025-09-03 23:17 ` [PATCH v4] " ノウラ | Flare via GitGitGadget
2025-09-04 7:47 ` Junio C Hamano
2025-09-04 13:25 ` ノウラ | Flare
2025-09-04 16:43 ` Junio C Hamano
2025-09-04 17:44 ` [PATCH v5] " ノウラ | Flare via GitGitGadget
2025-09-04 20:25 ` Junio C Hamano
2025-09-04 20:49 ` Jeff King
2025-09-04 22:26 ` Junio C Hamano
2025-09-05 0:02 ` ノウラ | Flare [this message]
2025-09-05 13:23 ` Jeff King
2025-09-05 17:27 ` ノウラ | Flare
2025-09-05 0:07 ` ノウラ | Flare
2025-09-05 0:25 ` ノウラ | Flare
2025-09-05 1:03 ` ノウラ | Flare
2025-09-05 14:39 ` Junio C Hamano
2025-09-05 17:47 ` ノウラ | Flare
2025-09-05 13:15 ` Jeff King
2025-09-05 18:51 ` [PATCH v6] " ノウラ | Flare via GitGitGadget
2025-09-05 19:37 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8f831259-7372-4357-b059-cc21f7a04864@gmail.com \
--to=nouraellm@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox