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

["René Scharfe" <l.s.r@web.de>]

On 9/12/26 12:09 AM, Junio C Hamano wrote:
> 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.
> 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é


> 
> 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;
>  @@