Re: [PATCH v3] alloc: fix dangling pointer in alloc_state cleanup
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 3, 2025, 21:59 UTC
- Message-ID
- <xmqqtt1jci4k.fsf@gitster.g>
- In-Reply-To
- <20250903111829.GA1877518@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> 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.