Re: [PATCH] cocci: remove risky "if (!E) free(E)" conversion

From: René Scharfe <l.s.r@web.de>
Date: Sep 12, 2026, 07:13 UTC
Message-ID: <caa39ca4-b35e-4fff-80fb-af6856cb2098@web.de>
In-Reply-To: <xmqqld978mok.fsf@gitster.g>

On 9/12/26 12:09 AM, Junio C Hamano wrote:

Show 18 quoted lines

> The current cocci patches try to convert
> 
> 	if (!E)
> 		free(E);
> 
> into an unconditional call to free(E), with the rationale
> 
>     cocci: detect useless free(3) calls
> 
>     Add a semantic patch for removing checks that cause free(3) to only be
>     called with a NULL pointer, as that must be a programming mistake.
> 
> which came from ec6cd14c7a (cocci: detect useless free(3) calls,
> 2017-02-11).
> 
> Leaving _something_ in ALL.patch output to draw programmers'
> attention is a good thing, but this changes a piece of code that is
> originally a no-op to do something else, which may be even worse.

Good point. It's likely that the programmer just wanted to release the object in question and got the check wrong, but it's also possible that the free(3) call is wrong as well, and that could do real damage.

Show 7 quoted lines

> We could change it to
> 
> 	if (!E)
> 		BUG("free(E) is certainly not what we meant to write");
> 
> to force programmers to think.  But it probably is safer to just
> rewrite one form of no-op into a simpler form of no-op.

With that last sentence I expected the patch to also remove the free(3) or commit_list_free() call, replacing the no-op with nothing, which is safe and simple.

On the other hand: Do we get any value out of this rule? Is it a useful guardrail? LeakSanitizer would find a forgotten free(3) call as well, given enough test coverage.

René

Show 27 quoted lines

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  tools/coccinelle/free.cocci | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/tools/coccinelle/free.cocci b/tools/coccinelle/free.cocci
> index 03799e1908..3dfaae9dd8 100644
> --- a/tools/coccinelle/free.cocci
> +++ b/tools/coccinelle/free.cocci
> @@ -8,16 +8,6 @@ expression E;
>    commit_list_free(E);
>  )
>  
> -@@
> -expression E;
> -@@
> -- if (!E)
> -(
> -  free(E);
> -|
> -  commit_list_free(E);
> -)
> -
>  @@
>  expression E;
>  @@