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

Thu Jan 14 00:34:43 PST 2016

I can confirm that unix.MallocSizeof, unix.cstring.BadSizeArg,
unix.cstring.NullArg, and unix.API, should also be turned on - they all
seem to work correctly.

As a matter of fact, should we just enable all of the unix checkers?

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 8:20 PM, <Alexander G. Riccio> <test35965 at gmail.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.
>
> 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/20160114/9f5db12e/attachment.html>