[cfe-dev] RFC: default to -Werror=format-security

Tue Feb 16 21:58:46 PST 2016

I've just noticed that there's no warning when printf has more than one
argument, is this a bug?

On Wed, Feb 17, 2016 at 9:47 AM, Craig, Ben via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> As a note on this, I'm only mildly in favor of making this an error by
> default.  The points raised by Nico are good, though they haven't convinced
> me to abandon the cause entirely :).  I won't be super upset if we keep
> this a warning.
>
> Regardless of the default-ness of this particular issue, I would really
> love to see a fixit for the trivial "printf(fmt);" case.
>
>
> On 2/16/2016 3:56 PM, Aaron Ballman wrote:
>
>> On Tue, Feb 16, 2016 at 4:26 PM, Nico Weber <thakis at chromium.org> wrote:
>>
>>> On Tue, Feb 16, 2016 at 3:59 PM, Aaron Ballman <aaron at aaronballman.com>
>>> wrote:
>>>
>>>> On Tue, Feb 16, 2016 at 3:44 PM, Nico Weber via cfe-dev
>>>> <cfe-dev at lists.llvm.org> wrote:
>>>>
>>>>> On Tue, Feb 16, 2016 at 12:22 PM, Craig, Ben <ben.craig at codeaurora.org
>>>>> >
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On 2/16/2016 1:18 PM, Nico Weber via cfe-dev wrote:
>>>>>>
>>>>>>>
>>>>>>> Won't this line of reasoning lead to all useful warnings being in
>>>>>>> -Werror
>>>>>>> eventually? Say, forgetting a return statement in a function is also
>>>>>>> "just"
>>>>>>> a warning...
>>>>>>>
>>>>>>
>>>>>> Not all of them :)
>>>>>>
>>>>>> Visual Studio groups warnings into big warning level buckets.  Level 1
>>>>>> has
>>>>>> the most important / severe (obvious use of uninitialized value),
>>>>>> level
>>>>>> 4
>>>>>> has fairly minor warnings (unused parameter), and /Weverything will
>>>>>> tell you
>>>>>> about really useless stuff (warning! you just used __declspec(align)!
>>>>>> ).  I
>>>>>> could imagine a world where the "Level 1", and maybe "Level 2"
>>>>>> warnings
>>>>>> were
>>>>>> errors by default.
>>>>>>
>>>>>
>>>>> We have this too: on-by-default warnings, -Wall, -Wextra, -Weverything.
>>>>> I
>>>>> don't think we should turn on-by-default warnings into errors.
>>>>>
>>>> We already do (in fact, we have over two dozen that we turn on,
>>>> according to the list posted earlier in the thread).
>>>>
>>>
>>> But that's for things that are supposed to be errors but can't be due to
>>> system headers, not the other way round.
>>>
>> Fair point.
>>
>> We should make it harder to compile broken code, and easier to write
>>>>>> correct code.  We can't change it all at once without angering the
>>>>>> world
>>>>>> though :)
>>>>>>
>>>>>
>>>>> People who don't like writing broken code don't ignore warnings. People
>>>>> who
>>>>> do like writing broken code will just pass -Wno-error. I don't think
>>>>> this
>>>>> proposal helps either party.
>>>>>
>>>> I think there's a reasonable distinction between "this code may not do
>>>> what you expect" and "this code may not do what you expect, and is
>>>> likely to result in security vulnerabilities."
>>>>
>>>
>>> "may not be what you expect" can always be a security vulnerability –
>>> think
>>> of -Wfor-loop-analysis, -Wunreachable, etc. We have warnings with much
>>> higher true positive rate than -Wformat-security.
>>>
>> Sorry, but printf(fmt); is *always* a true positive in my book. Same
>> with failing to return from all code paths. (etc)
>>
>> I do hear what you are saying and am not suggesting we should go about
>> this in a shotgun fashion. Each warning would require careful thought,
>> and possibly also require alterations to the diagnostic before turning
>> it into something that is an error by default.
>>
>> If someone elects to
>>>> turn on -Wno-error to get their insecure code to compile, there's
>>>> basically no help for them. But for projects that don't compile
>>>> particularly cleanly (so security-related diagnostics get lost in the
>>>> noise)
>>>>
>>>
>>> If our default warning set is too noisy, that's a bug. Every
>>> on-by-default
>>> warning should be super useful -- that's the bar for on-by-default
>>> warnings.
>>>
>> Super useful doesn't mean "is actually fixed by the developer." I have
>> been on projects that have a significant number of super useful
>> diagnostics that live in the source for *years* because of the sheer
>> quantity of diagnostics. When attempting to improve those kinds of
>> projects, they would get serious benefit from the notion of "some
>> diagnostics are more important than others." In fact, one such project
>> like that used Visual Studio's warning levels to do exactly that.
>> Unfortunately, clang has no such mechanism -- this sounds like a
>> proposal to add one. Perhaps the discussion should not be "warning vs
>> error" but instead about warning levels or some other mechanism to
>> call out the severity of a diagnostic. I want to get some utilities in
>> place that help existing code bases that are diagnostic-heavy get
>> security-related warnings under control.
>>
>> ~Aaron
>>
>> , being alerted, "no, really, this is bad" will definitely help
>>>> *some* programmers. I am interested in alerting people who might be
>>>> willing to fix security vulnerabilities in their code. I'm unconcerned
>>>> about people who will go to any lengths to get their code to compile
>>>> without actually fixing their code when it comes to security concerns.
>>>>
>>>> ~Aaron
>>>>
>>>>
>>>>>>
>>>>>> --
>>>>>> Employee of Qualcomm Innovation Center, Inc.
>>>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
>>>>>> Linux
>>>>>> Foundation Collaborative Project
>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> cfe-dev mailing list
>>>>> cfe-dev at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>
>>>>>
>>>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
>
> _______________________________________________
> 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/20160217/dcd29114/attachment.html>