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

[Craig, Ben via cfe-dev]

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