threads / patch / 49300

patchconfig.mak.dev: add -Wformat-security

Subject: [PATCH] config.mak.dev: add -Wformat-security

## tl;dr

3 messages between Sep 7, 2018 and Sep 8, 2018. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jeff King· Sep 7, 2018, 18:19 UTC · lore

We currently build cleanly with -Wformat-security, and it's a good idea to make sure we continue to do so (since calls that trigger the warning may be security vulnerabilities).

Note that we cannot use the stronger -Wformat-nonliteral, as there are case where we are clever with passing around pointers to string literals. E.g., bisect_rev_setup() takes bad_format and good_format parameters. These ultimately come from literals, but they still trigger the warning.

Some of these might be fixable (e.g., by passing flags from which we locally select a format), and might even be worth fixing (not because of security, but just because it's an easy mistake to pass the wrong format). But there are other cases which are likely quite hard to fix (we actually generate formats in a local buffer in some cases). So let's punt on that for now and start with -Wformat-security, which is supposed to catch the most important cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.mak.dev | 1 +
 1 file changed, 1 insertion(+)
Show changes to config.mak.dev +1 −0
diff --git a/config.mak.dev b/config.mak.dev
index 9a998149d9..f832752454 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -14,6 +14,7 @@ CFLAGS += -Wpointer-arith
 CFLAGS += -Wstrict-prototypes
 CFLAGS += -Wunused
 CFLAGS += -Wvla
+CFLAGS += -Wformat-security
 
 ifndef COMPILER_FEATURES
 COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
-- 
2.19.0.rc2.594.gc4806cd463
Duy Nguyen· Sep 8, 2018, 13:38 UTC · re: Jeff King · lore

Re: [PATCH] config.mak.dev: add -Wformat-security

On Fri, Sep 7, 2018 at 8:21 PM Jeff King <peff@peff.net> wrote:
>
> We currently build cleanly with -Wformat-security, and it's
> a good idea to make sure we continue to do so (since calls
> that trigger the warning may be security vulnerabilities).

Nice. I had this flag in my config.mak too before switching to DEVELOPER=1. Didn't realize I lost the flag until now.

Show 14 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  config.mak.dev | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 9a998149d9..f832752454 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -14,6 +14,7 @@ CFLAGS += -Wpointer-arith
>  CFLAGS += -Wstrict-prototypes
>  CFLAGS += -Wunused
>  CFLAGS += -Wvla
> +CFLAGS += -Wformat-security
Maybe keep it sorted
-- 
Duy
Jeff King· Sep 8, 2018, 16:23 UTC · re: Duy Nguyen · lore

[PATCH v2] config.mak.dev: add -Wformat-security

On Sat, Sep 08, 2018 at 03:38:19PM +0200, Duy Nguyen wrote:
Show 8 quoted lines
> On Fri, Sep 7, 2018 at 8:21 PM Jeff King <peff@peff.net> wrote:
> >
> > We currently build cleanly with -Wformat-security, and it's
> > a good idea to make sure we continue to do so (since calls
> > that trigger the warning may be security vulnerabilities).
> 
> Nice. I had this flag in my config.mak too before switching to
> DEVELOPER=1. Didn't realize I lost the flag until now.

I thought I used to use it, too, but I realized recently that I never did (I experimented with format-nonliteral a long time ago but gave up).

AFAIK we've never actually had a case that this triggered, because it's so narrow (as opposed to format-nonliteral).

Show 11 quoted lines
> > diff --git a/config.mak.dev b/config.mak.dev
> > index 9a998149d9..f832752454 100644
> > --- a/config.mak.dev
> > +++ b/config.mak.dev
> > @@ -14,6 +14,7 @@ CFLAGS += -Wpointer-arith
> >  CFLAGS += -Wstrict-prototypes
> >  CFLAGS += -Wunused
> >  CFLAGS += -Wvla
> > +CFLAGS += -Wformat-security
> 
> Maybe keep it sorted

Heh, I didn't even notice the original was sorted. Here it is in the right spot:

-- >8 --
Subject: config.mak.dev: add -Wformat-security

We currently build cleanly with -Wformat-security, and it's a good idea to make sure we continue to do so (since calls that trigger the warning may be security vulnerabilities).

Note that we cannot use the stronger -Wformat-nonliteral, as there are case where we are clever with passing around pointers to string literals. E.g., bisect_rev_setup() takes bad_format and good_format parameters. These ultimately come from literals, but they still trigger the warning.

Some of these might be fixable (e.g., by passing flags from which we locally select a format), and might even be worth fixing (not because of security, but just because it's an easy mistake to pass the wrong format). But there are other cases which are likely quite hard to fix (we actually generate formats in a local buffer in some cases). So let's punt on that for now and start with -Wformat-security, which is supposed to catch the most important cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.mak.dev | 1 +
 1 file changed, 1 insertion(+)
Show changes to config.mak.dev +1 −0
diff --git a/config.mak.dev b/config.mak.dev
index 9a998149d9..92d268137f 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -7,6 +7,7 @@ CFLAGS += -pedantic
 CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0
 endif
 CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wformat-security
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
 CFLAGS += -Woverflow
-- 
2.19.0.rc2.594.gc4806cd463

← back to recent threads