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

[Aaron Ballman via cfe-dev]

On Wed, Jan 13, 2016 at 12:57 AM, Yury Gribov via cfe-dev
<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.

~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