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:25:55 +0200 [thread overview]
Message-ID: <88ae6c7b-0a69-48bd-9e73-672331a22197@gmail.com> (raw)
In-Reply-To: <52fb3fea-4842-4532-8572-df41537da5f9@gmail.com>
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);
>> }
next prev parent reply other threads:[~2025-09-05 0:25 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
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 [this message]
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=88ae6c7b-0a69-48bd-9e73-672331a22197@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