{"thread":{"id":"49300","subject":"[PATCH] config.mak.dev: add -Wformat-security","startedAt":"2018-09-07T18:19:09Z","lastAt":"2018-09-08T16:23:34Z","messageCount":3,"participants":["Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"357640","messageId":"20180907181905.GA15897@sigill.intra.peff.net","threadId":"49300","inReplyTo":null,"subject":"[PATCH] config.mak.dev: add -Wformat-security","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-07T18:19:05Z","receivedAt":"2018-09-07T18:19:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently build cleanly with -Wformat-security, and it's\na good idea to make sure we continue to do so (since calls\nthat trigger the warning may be security vulnerabilities).\n\nNote that we cannot use the stronger -Wformat-nonliteral, as\nthere are case where we are clever with passing around\npointers to string literals. E.g., bisect_rev_setup() takes\nbad_format and good_format parameters. These ultimately come\nfrom literals, but they still trigger the warning.\n\nSome of these might be fixable (e.g., by passing flags from\nwhich we locally select a format), and might even be worth\nfixing (not because of security, but just because it's an\neasy mistake to pass the wrong format). But there are other\ncases which are likely quite hard to fix (we actually\ngenerate formats in a local buffer in some cases). So let's\npunt on that for now and start with -Wformat-security, which\nis supposed to catch the most important cases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.mak.dev | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 9a998149d9..f832752454 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -14,6 +14,7 @@ CFLAGS += -Wpointer-arith\n CFLAGS += -Wstrict-prototypes\n CFLAGS += -Wunused\n CFLAGS += -Wvla\n+CFLAGS += -Wformat-security\n \n ifndef COMPILER_FEATURES\n COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n-- \n2.19.0.rc2.594.gc4806cd463\n"},{"id":"357691","messageId":"CACsJy8CshOFNgHq-QQZp3GM3Ua+iuKMDvA+YaKKh1QGF5FrHqg@mail.gmail.com","threadId":"49300","inReplyTo":"20180907181905.GA15897@sigill.intra.peff.net","subject":"Re: [PATCH] config.mak.dev: add -Wformat-security","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-08T13:38:19Z","receivedAt":"2018-09-08T13:38:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Sep 7, 2018 at 8:21 PM Jeff King <peff@peff.net> wrote:\n>\n> We currently build cleanly with -Wformat-security, and it's\n> a good idea to make sure we continue to do so (since calls\n> that trigger the warning may be security vulnerabilities).\n\nNice. I had this flag in my config.mak too before switching to\nDEVELOPER=1. Didn't realize I lost the flag until now.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  config.mak.dev | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/config.mak.dev b/config.mak.dev\n> index 9a998149d9..f832752454 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -14,6 +14,7 @@ CFLAGS += -Wpointer-arith\n>  CFLAGS += -Wstrict-prototypes\n>  CFLAGS += -Wunused\n>  CFLAGS += -Wvla\n> +CFLAGS += -Wformat-security\n\nMaybe keep it sorted\n-- \nDuy\n"},{"id":"357699","messageId":"20180908162331.GB9016@sigill.intra.peff.net","threadId":"49300","inReplyTo":"CACsJy8CshOFNgHq-QQZp3GM3Ua+iuKMDvA+YaKKh1QGF5FrHqg@mail.gmail.com","subject":"[PATCH v2] config.mak.dev: add -Wformat-security","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-08T16:23:31Z","receivedAt":"2018-09-08T16:23:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 08, 2018 at 03:38:19PM +0200, Duy Nguyen wrote:\n\n> On Fri, Sep 7, 2018 at 8:21 PM Jeff King <peff@peff.net> wrote:\n> >\n> > We currently build cleanly with -Wformat-security, and it's\n> > a good idea to make sure we continue to do so (since calls\n> > that trigger the warning may be security vulnerabilities).\n> \n> Nice. I had this flag in my config.mak too before switching to\n> DEVELOPER=1. Didn't realize I lost the flag until now.\n\nI thought I used to use it, too, but I realized recently that I never\ndid (I experimented with format-nonliteral a long time ago but gave up).\n\nAFAIK we've never actually had a case that this triggered, because it's\nso narrow (as opposed to format-nonliteral).\n\n> > diff --git a/config.mak.dev b/config.mak.dev\n> > index 9a998149d9..f832752454 100644\n> > --- a/config.mak.dev\n> > +++ b/config.mak.dev\n> > @@ -14,6 +14,7 @@ CFLAGS += -Wpointer-arith\n> >  CFLAGS += -Wstrict-prototypes\n> >  CFLAGS += -Wunused\n> >  CFLAGS += -Wvla\n> > +CFLAGS += -Wformat-security\n> \n> Maybe keep it sorted\n\nHeh, I didn't even notice the original was sorted. Here it is in the\nright spot:\n\n-- >8 --\nSubject: config.mak.dev: add -Wformat-security\n\nWe currently build cleanly with -Wformat-security, and it's\na good idea to make sure we continue to do so (since calls\nthat trigger the warning may be security vulnerabilities).\n\nNote that we cannot use the stronger -Wformat-nonliteral, as\nthere are case where we are clever with passing around\npointers to string literals. E.g., bisect_rev_setup() takes\nbad_format and good_format parameters. These ultimately come\nfrom literals, but they still trigger the warning.\n\nSome of these might be fixable (e.g., by passing flags from\nwhich we locally select a format), and might even be worth\nfixing (not because of security, but just because it's an\neasy mistake to pass the wrong format). But there are other\ncases which are likely quite hard to fix (we actually\ngenerate formats in a local buffer in some cases). So let's\npunt on that for now and start with -Wformat-security, which\nis supposed to catch the most important cases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.mak.dev | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 9a998149d9..92d268137f 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -7,6 +7,7 @@ CFLAGS += -pedantic\n CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0\n endif\n CFLAGS += -Wdeclaration-after-statement\n+CFLAGS += -Wformat-security\n CFLAGS += -Wno-format-zero-length\n CFLAGS += -Wold-style-definition\n CFLAGS += -Woverflow\n-- \n2.19.0.rc2.594.gc4806cd463\n\n"}]}