{"thread":{"id":"66312","subject":"[PATCH] cocci: remove risky \"if (!E) free(E)\" conversion","startedAt":"2026-09-11T22:09:49Z","lastAt":"2026-09-13T10:46:00Z","messageCount":5,"participants":["Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"552610","messageId":"xmqqld978mok.fsf@gitster.g","threadId":"66312","inReplyTo":null,"subject":"[PATCH] cocci: remove risky \"if (!E) free(E)\" conversion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-11T22:09:47Z","receivedAt":"2026-09-11T22:09:49Z","isPatch":true,"body":"The current cocci patches try to convert\n\n\tif (!E)\n\t\tfree(E);\n\ninto an unconditional call to free(E), with the rationale\n\n cocci: detect useless free(3) calls\n\n Add a semantic patch for removing checks that cause free(3) to only be\n called with a NULL pointer, as that must be a programming mistake.\n\nwhich came from ec6cd14c7a (cocci: detect useless free(3) calls,\n2017-02-11).\n\nLeaving _something_ in ALL.patch output to draw programmers'\nattention is a good thing, but this changes a piece of code that is\noriginally a no-op to do something else, which may be even worse.\n\nWe could change it to\n\n\tif (!E)\n\t\tBUG(\"free(E) is certainly not what we meant to write\");\n\nto force programmers to think. But it probably is safer to just\nrewrite one form of no-op into a simpler form of no-op.\n\nSigned-off-by: Junio C Hamano \n---\n tools/coccinelle/free.cocci | 10 ----------\n 1 file changed, 10 deletions(-)\n\ndiff --git a/tools/coccinelle/free.cocci b/tools/coccinelle/free.cocci\nindex 03799e1908..3dfaae9dd8 100644\n--- a/tools/coccinelle/free.cocci\n+++ b/tools/coccinelle/free.cocci\n@@ -8,16 +8,6 @@ expression E;\n commit_list_free(E);\n )\n \n-@@\n-expression E;\n-@@\n-- if (!E)\n-(\n- free(E);\n-|\n- commit_list_free(E);\n-)\n-\n @@\n expression E;\n @@\n-- \n2.56.0-rc0-143-g1fea62d0ca\n\n"},{"id":"552611","messageId":"xmqqh5jv8m4w.fsf@gitster.g","threadId":"66312","inReplyTo":"xmqqld978mok.fsf@gitster.g","subject":"[PATCH] cocci: FREE_AND_NULL(E) is safe to call on NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-11T22:21:35Z","receivedAt":"2026-09-11T22:21:37Z","isPatch":true,"body":"Just like we allow calling free(E) without checking if E is not\nNULL, it is safe to call FREE_AND_NULL(E) unconditionally.\n\nSigned-off-by: Junio C Hamano \n---\n tools/coccinelle/free.cocci | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/tools/coccinelle/free.cocci b/tools/coccinelle/free.cocci\nindex 3dfaae9dd8..f2af140cfb 100644\n--- a/tools/coccinelle/free.cocci\n+++ b/tools/coccinelle/free.cocci\n@@ -6,6 +6,8 @@ expression E;\n free(E);\n |\n commit_list_free(E);\n+|\n+ FREE_AND_NULL(E);\n )\n \n @@\n-- \n2.56.0-rc0-143-g1fea62d0ca\n\n"},{"id":"552619","messageId":"caa39ca4-b35e-4fff-80fb-af6856cb2098@web.de","threadId":"66312","inReplyTo":"xmqqld978mok.fsf@gitster.g","subject":"Re: [PATCH] cocci: remove risky \"if (!E) free(E)\" conversion","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-09-12T07:13:29Z","receivedAt":"2026-09-12T07:13:31Z","isPatch":true,"body":"On 9/12/26 12:09 AM, Junio C Hamano wrote:\n> The current cocci patches try to convert\n> \n> \tif (!E)\n> \t\tfree(E);\n> \n> into an unconditional call to free(E), with the rationale\n> \n> cocci: detect useless free(3) calls\n> \n> Add a semantic patch for removing checks that cause free(3) to only be\n> called with a NULL pointer, as that must be a programming mistake.\n> \n> which came from ec6cd14c7a (cocci: detect useless free(3) calls,\n> 2017-02-11).\n> \n> Leaving _something_ in ALL.patch output to draw programmers'\n> attention is a good thing, but this changes a piece of code that is\n> originally a no-op to do something else, which may be even worse.\n\nGood point. It's likely that the programmer just wanted to release the\nobject in question and got the check wrong, but it's also possible that\nthe free(3) call is wrong as well, and that could do real damage.\n> We could change it to\n> \n> \tif (!E)\n> \t\tBUG(\"free(E) is certainly not what we meant to write\");\n> \n> to force programmers to think. But it probably is safer to just\n> rewrite one form of no-op into a simpler form of no-op.\n\nWith that last sentence I expected the patch to also remove the free(3)\nor commit_list_free() call, replacing the no-op with nothing, which is\nsafe and simple.\n\nOn the other hand: Do we get any value out of this rule? Is it a\nuseful guardrail? LeakSanitizer would find a forgotten free(3) call as\nwell, given enough test coverage.\n\nRené\n\n\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> tools/coccinelle/free.cocci | 10 ----------\n> 1 file changed, 10 deletions(-)\n> \n> diff --git a/tools/coccinelle/free.cocci b/tools/coccinelle/free.cocci\n> index 03799e1908..3dfaae9dd8 100644\n> --- a/tools/coccinelle/free.cocci\n> +++ b/tools/coccinelle/free.cocci\n> @@ -8,16 +8,6 @@ expression E;\n> commit_list_free(E);\n> )\n> \n> -@@\n> -expression E;\n> -@@\n> -- if (!E)\n> -(\n> - free(E);\n> -|\n> - commit_list_free(E);\n> -)\n> -\n> @@\n> expression E;\n> @@\n\n"},{"id":"552626","messageId":"xmqqcxui8f0c.fsf@gitster.g","threadId":"66312","inReplyTo":"caa39ca4-b35e-4fff-80fb-af6856cb2098@web.de","subject":"Re: [PATCH] cocci: remove risky \"if (!E) free(E)\" conversion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-12T19:07:47Z","receivedAt":"2026-09-12T19:07:50Z","isPatch":true,"body":"René Scharfe writes:\n\n> On 9/12/26 12:09 AM, Junio C Hamano wrote:\n>> The current cocci patches try to convert\n>> \n>> \tif (!E)\n>> \t\tfree(E);\n>> \n>> into an unconditional call to free(E), with the rationale\n>> \n>> cocci: detect useless free(3) calls\n>> \n>> Add a semantic patch for removing checks that cause free(3) to only be\n>> called with a NULL pointer, as that must be a programming mistake.\n>> \n>> which came from ec6cd14c7a (cocci: detect useless free(3) calls,\n>> 2017-02-11).\n>> \n>> Leaving _something_ in ALL.patch output to draw programmers'\n>> attention is a good thing, but this changes a piece of code that is\n>> originally a no-op to do something else, which may be even worse.\n>\n> Good point. It's likely that the programmer just wanted to release the\n> object in question and got the check wrong, but it's also possible that\n> the free(3) call is wrong as well, and that could do real damage.\n>> We could change it to\n>> \n>> \tif (!E)\n>> \t\tBUG(\"free(E) is certainly not what we meant to write\");\n>> \n>> to force programmers to think. But it probably is safer to just\n>> rewrite one form of no-op into a simpler form of no-op.\n>\n> With that last sentence I expected the patch to also remove the free(3)\n> or commit_list_free() call, replacing the no-op with nothing, which is\n> safe and simple.\n\nYou mean\n\n\t if (!E)\n\t- free(E);\n\t+ ; /* no op free(E) */\n\nor something? I guess we could do so, but I feared that a compiler\nthat is smart enough complain and trip -Werror on us when E is too\nobviously a side-effect free expression such as a reference to a\nsimple variable.\n"},{"id":"552640","messageId":"96aca004-0df4-4e21-b60a-0288239122cc@web.de","threadId":"66312","inReplyTo":"xmqqcxui8f0c.fsf@gitster.g","subject":"Re: [PATCH] cocci: remove risky \"if (!E) free(E)\" conversion","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-09-13T10:45:48Z","receivedAt":"2026-09-13T10:46:00Z","isPatch":true,"body":"On 9/12/26 9:07 PM, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n>>> We could change it to\n>>>\n>>> \tif (!E)\n>>> \t\tBUG(\"free(E) is certainly not what we meant to write\");\n>>>\n>>> to force programmers to think. But it probably is safer to just\n>>> rewrite one form of no-op into a simpler form of no-op.\n>>\n>> With that last sentence I expected the patch to also remove the free(3)\n>> or commit_list_free() call, replacing the no-op with nothing, which is\n>> safe and simple.\n> \n> You mean\n> \n> \t if (!E)\n> \t- free(E);\n> \t+ ; /* no op free(E) */\n> \n> or something? I guess we could do so, but I feared that a compiler\n> that is smart enough complain and trip -Werror on us when E is too\n> obviously a side-effect free expression such as a reference to a\n> simple variable.\n\nGCC apparently accepts \"if (!E);\", Clang warns. Both currently accept\n\"if (!E) {}\". See https://godbolt.org/z/9h8YdjrGP for some more\nvariants. Other compilers or versions could react differently, of\ncourse.\n\nI would have just removed everything:\n\n - if (!E) free(E);\n\n, risking the loss of side-effects and welcoming any warnings,\naccepting that this bluntness would be rude and potentially unsafe.\nThat's a bit like in the \"computer says no\" skits, I realize now.\n\nI agree that the polite thing to do is to leave the flawed code in and\nlet the programmer find out that it's not doing anything some other way.\nNo need to put up a targeted defense against this inconsequential and\nunlikely mistake.\n\nRené\n\n"}]}