[cfe-dev] Clang on Windows fails to detect trivial double free instatic analysis

Tue Jan 12 17:20:45 PST 2016

> My understanding is that this is in the “unix” package for historical
reasons from before the analyzer thought about Windows. We have clients
that rely on the malloc checker being enabled/disabled with ‘unix.Malloc'
so moving it will break compatibility.

Gahh, ok. Not breaking things is a good reason.

> I discussed this off-list with Anna Zaks. She recommended changing the
driver to enable unix.Malloc explicitly when running in a Windows MSVC
environment. This is Clang::ConstructJob() in Tools.cpp. We currently skip
enabling the “unix” package there for Windows MSVC.

Yup, that'd be a good short term fix.

> Someone who develops on Windows should write a patch and test it.
Alexander: would you be willing to do this? It should be very
straightforward — there is similar code to disable specific checkers for
PS4.

Yup! I could very well do that. I think the change would look something
like:

if (!IsWindowsMSVC)
        CmdArgs.push_back("-analyzer-checker=unix.Malloc");

> We should also add a comment Checkers.td to indicate that we should move
the Malloc checker to another package when we do remap packages names and
break backward compatibility. “core” is probably not the right place for
this (since malloc() is in the standard library). Maybe a new “cstdlib”
package?

(that's what I meant by "short term fix")
"cstdlib" makes more sense - core was just the first thing that came to
mind - and we could make cstdlib.malloc/cstdlib.Malloc alias unix.Malloc to
avoid breaking users of unix.Malloc.

Sidenote: has anybody ever considered diagnosing incorrectly capitalized
checker name arguments? It's not terribly important, but I was quite
annoyed to find out that I'd spent a couple hours debugging a lowercase "M".

Sincerely,
Alexander Riccio
--
"Change the world or go home."
about.me/ariccio

<http://about.me/ariccio>
If left to my own devices, I will build more.
⁂

On Tue, Jan 12, 2016 at 7:06 PM, Devin Coughlin <dcoughlin at apple.com> wrote:

> My understanding is that this is in the “unix” package for historical
> reasons from before the analyzer thought about Windows. We have clients
> that rely on the malloc checker being enabled/disabled with ‘unix.Malloc'
> so moving it will break compatibility.
>
> I discussed this off-list with Anna Zaks. She recommended changing the
> driver to enable unix.Malloc explicitly when running in a Windows MSVC
> environment. This is Clang::ConstructJob() in Tools.cpp. We currently skip
> enabling the “unix” package there for Windows MSVC.
>
> Someone who develops on Windows should write a patch and test it.
> Alexander: would you be willing to do this? It should be very
> straightforward — there is similar code to disable specific checkers for
> PS4.
>
> We should also add a comment Checkers.td to indicate that we should move
> the Malloc checker to another package when we do remap packages names and
> break backward compatibility. “core” is probably not the right place for
> this (since malloc() is in the standard library). Maybe a new “cstdlib”
> package?
>
> Devin
>
> On Jan 12, 2016, at 2:22 PM, Alexander Riccio via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> Shoot - I haven't responded to this. I did some debugging the other day
> and found that if I manually pass the flag to enable the unix.Malloc
> checker (that's a capital "M", as I discovered the hard way), Clang detects
> this.
>
> I was going to suggest something like enabling it by default (obvious),
> and *maybe* renaming it to something like core.Malloc, because it's not
> unix-specific.
>
> The one benefit here of parsing SAL is a more generic mechanism, but
> that's a different issue.
>
> sent from my (stupid) windows phone
> ------------------------------
> From: Reid Kleckner <rnk at google.com>
> Sent: ‎1/‎12/‎2016 5:18 PM
> To: <Alexander G. Riccio> <test35965 at gmail.com>; Jordan Rose
> <jordan_rose at apple.com>
> Cc: cfe-dev <cfe-dev at lists.llvm.org>
> Subject: Re: [cfe-dev] Clang on Windows fails to detect trivial double
> free instatic analysis
>
> Jordan, how do we enable this checker on Windows?
>
> We shouldn't need to be able to parse SAL to do this analysis.
>
> On Sun, Jan 3, 2016 at 10:31 PM, <Alexander G. Riccio> via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Is it because the checker is *unix*.malloc
>> <http://clang-analyzer.llvm.org/available_checks.html#unix_checkers>? If
>> so, that's actually quite terrible... why only check it on unix??
>>
>> Sincerely,
>> Alexander Riccio
>> --
>> "Change the world or go home."
>> about.me/ariccio
>>
>> <http://about.me/ariccio>
>> If left to my own devices, I will build more.
>> ⁂
>>
>> On Sat, Jan 2, 2016 at 3:57 PM, <Alexander G. Riccio> <
>> test35965 at gmail.com> wrote:
>>
>>> When I build the attached C program in windows, using Clang built from a
>>> very recent tree version (trunk 256686), Clang fails to detect the trivial
>>> double free, as evidenced in the resulting plist file (attached).
>>>
>>> What's going on here? I have a gut feeling that it has something to do
>>> with Clang's ignorance of SAL, which allows MSVC to detect the condition
>>> generically:
>>>
>>> void __cdecl free(
>>>     _Pre_maybenull_ _Post_invalid_ void* _Block
>>>     );
>>>
>>> (from C:/Program Files (x86)/Windows
>>> Kits/10/Include/10.0.10240.0/ucrt/corecrt_malloc.h)
>>>
>>> I'm also attaching the verbose compilation output.
>>>
>>> Sincerely,
>>> Alexander Riccio
>>> --
>>> "Change the world or go home."
>>> about.me/ariccio
>>>
>>> <http://about.me/ariccio>
>>> If left to my own devices, I will build more.
>>> ⁂
>>>
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160112/7c989d2b/attachment.html>