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

From: Jeff King <peff@peff.net>
Date: Sep 5, 2025, 13:23 UTC
Message-ID: <20250905132309.GC596956@coredump.intra.peff.net>
In-Reply-To: <8f831259-7372-4357-b059-cc21f7a04864@gmail.com>

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

Show 13 quoted lines

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

Show 11 quoted lines

> 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