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

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

[Junio C Hamano]

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.

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.

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;
 @@
-- 
2.56.0-rc0-143-g1fea62d0ca

[PATCH] cocci: FREE_AND_NULL(E) is safe to call on NULL

[Junio C Hamano]

Just like we allow calling free(E) without checking if E is not
NULL, it is safe to call FREE_AND_NULL(E) unconditionally.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 tools/coccinelle/free.cocci | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/coccinelle/free.cocci b/tools/coccinelle/free.cocci
index 3dfaae9dd8..f2af140cfb 100644
--- a/tools/coccinelle/free.cocci
+++ b/tools/coccinelle/free.cocci
@@ -6,6 +6,8 @@ expression E;
   free(E);
 |
   commit_list_free(E);
+|
+  FREE_AND_NULL(E);
 )
 
 @@
-- 
2.56.0-rc0-143-g1fea62d0ca

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

[René Scharfe]

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

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

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

You mean

	 if (!E)
	-  free(E);
	+  ; /* no op free(E) */

or something?  I guess we could do so, but I feared that a compiler
that is smart enough complain and trip -Werror on us when E is too
obviously a side-effect free expression such as a reference to a
simple variable.

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

[René Scharfe]

On 9/12/26 9:07 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>>> 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.
> 
> You mean
> 
> 	 if (!E)
> 	-  free(E);
> 	+  ; /* no op free(E) */
> 
> or something?  I guess we could do so, but I feared that a compiler
> that is smart enough complain and trip -Werror on us when E is too
> obviously a side-effect free expression such as a reference to a
> simple variable.

GCC apparently accepts "if (!E);", Clang warns.  Both currently accept
"if (!E) {}". See https://godbolt.org/z/9h8YdjrGP for some more
variants.  Other compilers or versions could react differently, of
course.

I would have just removed everything:

   -  if (!E) free(E);

, risking the loss of side-effects and welcoming any warnings,
accepting that this bluntness would be rude and potentially unsafe.
That's a bit like in the "computer says no" skits, I realize now.

I agree that the polite thing to do is to leave the flawed code in and
let the programmer find out that it's not doing anything some other way.
No need to put up a targeted defense against this inconsequential and
unlikely mistake.

René