# [PATCH] cocci: remove risky "if (!E) free(E)" conversion 5 messages from 2026-09-11 to 2026-09-13. Participants: Junio C Hamano, René Scharfe. Thread: https://gitlist.dev/t/66312 ## Junio C Hamano, 2026-09-11 22:09 Subject: [PATCH] cocci: remove risky "if (!E) free(E)" conversion Message-ID: ``` 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 --- 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 ``` ## Junio C Hamano, 2026-09-11 22:21 Subject: [PATCH] cocci: FREE_AND_NULL(E) is safe to call on NULL Message-ID: In-Reply-To: ``` 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 --- 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 ``` ## René Scharfe, 2026-09-12 07:13 Subject: Re: [PATCH] cocci: remove risky "if (!E) free(E)" conversion Message-ID: In-Reply-To: ``` 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 > --- > 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; > @@ ``` ## Junio C Hamano, 2026-09-12 19:07 Subject: Re: [PATCH] cocci: remove risky "if (!E) free(E)" conversion Message-ID: In-Reply-To: ``` René Scharfe 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. ``` ## René Scharfe, 2026-09-13 10:45 Subject: Re: [PATCH] cocci: remove risky "if (!E) free(E)" conversion Message-ID: <96aca004-0df4-4e21-b60a-0288239122cc@web.de> In-Reply-To: ``` On 9/12/26 9:07 PM, Junio C Hamano wrote: > René Scharfe 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é ```