Re: [PATCH] cocci: remove risky "if (!E) free(E)" conversion
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 12, 2026, 19:07 UTC
- Message-ID
- <xmqqcxui8f0c.fsf@gitster.g>
- In-Reply-To
- <caa39ca4-b35e-4fff-80fb-af6856cb2098@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 34 quoted lines
> 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.