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

Anna Zaks via cfe-dev cfe-dev at lists.llvm.org
Tue Jan 19 17:40:25 PST 2016 

> On Jan 14, 2016, at 5:26 AM, Aaron Ballman via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
> On Wed, Jan 13, 2016 at 12:57 AM, Yury Gribov via cfe-dev
> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>> On 01/13/2016 04:20 AM, <Alexander G. Riccio> via cfe-dev 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.
>>> 
>>> Gahh, ok. Not breaking things is a good reason.
>> 
>> 
>> Why not implement checker synonyms? This sounds like a general problem.
> 
> This is what clang-tidy did for its checkers, and it seems like the
> static analyzer could maybe benefit from the same feature. I can
> foresee use for such a thing if, for instance, people wanted to add
> static analysis checks for various coding guidelines that have
> overlapping requirements.
> 

This would definitely be valuable.

> ~Aaron
> 
>> 
>>>> 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
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> 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
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev <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/20160119/fad7210e/attachment.html>

More information about the cfe-dev mailing list