Bug 1320175 - "-Werror=format-nonliteral" failure but worked in F23
Summary: "-Werror=format-nonliteral" failure but worked in F23
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: 24
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-03-22 14:00 UTC by Bastien Nocera
Modified: 2016-03-22 15:51 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2016-03-22 15:16:02 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Bastien Nocera 2016-03-22 14:00:37 UTC
Trying to build grilo-plugins 0.3.1 in Fedora 24 and rawhide fails:

make[3]: Entering directory '/builddir/build/BUILD/grilo-plugins-0.3.1/src/jamendo'
  CC       libgrljamendo_la-grl-jamendo.lo
grl-jamendo.c: In function 'grl_jamendo_source_browse':
grl-jamendo.c:1180:32: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
                                page_number);
                                ^~~~~~~~~~~
grl-jamendo.c: In function 'grl_jamendo_source_query':
grl-jamendo.c:1281:26: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
                          term);
                          ^~~~
with gcc-6.0.0-0.15.fc24

Compiling on F23 with gcc-5.3.1-2.fc23.x86_64 works:
V=1 make
/bin/sh ../../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I../..    -pthread -I/home/hadess/Projects/gnome-install/include/grilo-0.3 -I/home/hadess/Projects/gnome-install/include/glib-2.0 -I/home/hadess/Projects/gnome-install/lib/glib-2.0/include -I/usr/include/libxml2  -pthread -I/home/hadess/Projects/gnome-install/include/grilo-0.3 -I/home/hadess/Projects/gnome-install/include/glib-2.0 -I/home/hadess/Projects/gnome-install/lib/glib-2.0/include -I/usr/include/libxml2  -I/usr/include/libxml2  -DG_LOG_DOMAIN=\"GrlJamendo\" -DLOCALEDIR=\"/home/hadess/Projects/gnome-install/share/locale\" -g3 -ggdb -Wformat -Werror=format-nonliteral  -Wall -Wstrict-prototypes -Wnested-externs -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=pointer-arith -Werror=init-self -Werror=format-security -Werror=format=2 -Werror=missing-include-dirs -Werror=return-type  -Wmissing-declarations -std=c99  -MT libgrljamendo_la-grl-jamendo.lo -MD -MP -MF .deps/libgrljamendo_la-grl-jamendo.Tpo -c -o libgrljamendo_la-grl-jamendo.lo `test -f 'grl-jamendo.c' || echo './'`grl-jamendo.c
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I../.. -pthread -I/home/hadess/Projects/gnome-install/include/grilo-0.3 -I/home/hadess/Projects/gnome-install/include/glib-2.0 -I/home/hadess/Projects/gnome-install/lib/glib-2.0/include -I/usr/include/libxml2 -pthread -I/home/hadess/Projects/gnome-install/include/grilo-0.3 -I/home/hadess/Projects/gnome-install/include/glib-2.0 -I/home/hadess/Projects/gnome-install/lib/glib-2.0/include -I/usr/include/libxml2 -I/usr/include/libxml2 -DG_LOG_DOMAIN=\"GrlJamendo\" -DLOCALEDIR=\"/home/hadess/Projects/gnome-install/share/locale\" -g3 -ggdb -Wformat -Werror=format-nonliteral -Wall -Wstrict-prototypes -Wnested-externs -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=pointer-arith -Werror=init-self -Werror=format-security -Werror=format=2 -Werror=missing-include-dirs -Werror=return-type -Wmissing-declarations -std=c99 -MT libgrljamendo_la-grl-jamendo.lo -MD -MP -MF .deps/libgrljamendo_la-grl-jamendo.Tpo -c grl-jamendo.c  -fPIC -DPIC -o .libs/libgrljamendo_la-grl-jamendo.o
mv -f .deps/libgrljamendo_la-grl-jamendo.Tpo .deps/libgrljamendo_la-grl-jamendo.Plo
/bin/sh ../../libtool  --tag=CC   --mode=link gcc -pthread -I/home/hadess/Projects/gnome-install/include/grilo-0.3 -I/home/hadess/Projects/gnome-install/include/glib-2.0 -I/home/hadess/Projects/gnome-install/lib/glib-2.0/include -I/usr/include/libxml2  -pthread -I/home/hadess/Projects/gnome-install/include/grilo-0.3 -I/home/hadess/Projects/gnome-install/include/glib-2.0 -I/home/hadess/Projects/gnome-install/lib/glib-2.0/include -I/usr/include/libxml2  -I/usr/include/libxml2  -DG_LOG_DOMAIN=\"GrlJamendo\" -DLOCALEDIR=\"/home/hadess/Projects/gnome-install/share/locale\" -g3 -ggdb -Wformat -Werror=format-nonliteral  -Wall -Wstrict-prototypes -Wnested-externs -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=pointer-arith -Werror=init-self -Werror=format-security -Werror=format=2 -Werror=missing-include-dirs -Werror=return-type  -Wmissing-declarations -std=c99  -no-undefined -module -avoid-version -L/home/hadess/Projects/gnome-install/lib  -o libgrljamendo.la -rpath /home/hadess/Projects/gnome-install/lib/grilo-0.3 libgrljamendo_la-grl-jamendo.lo -L/home/hadess/Projects/gnome-install/lib -lgrilo-0.3 -Wl,--export-dynamic -lgmodule-2.0 -pthread -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lxml2  -L/home/hadess/Projects/gnome-install/lib -lgrlnet-0.3 -lgrilo-0.3 -Wl,--export-dynamic -lgmodule-2.0 -pthread -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lxml2  -lxml2  
libtool: link: gcc -shared  -fPIC -DPIC  .libs/libgrljamendo_la-grl-jamendo.o   -Wl,-rpath -Wl,/home/hadess/Projects/gnome-install/lib -Wl,-rpath -Wl,/home/hadess/Projects/gnome-install/lib -L/home/hadess/Projects/gnome-install/lib -lgrlnet-0.3 /home/hadess/Projects/gnome-install/lib/libgrilo-0.3.so -lgmodule-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lxml2  -pthread -pthread -g3 -ggdb -Wl,--export-dynamic -pthread -Wl,--export-dynamic -pthread   -pthread -Wl,-soname -Wl,libgrljamendo.so -o .libs/libgrljamendo.so
libtool: link: ( cd ".libs" && rm -f "libgrljamendo.la" && ln -s "../libgrljamendo.la" "libgrljamendo.la" )

The code for that file is here:
https://git.gnome.org/browse/grilo-plugins/tree/src/jamendo/grl-jamendo.c

And while the code around line 1281 is easy enough to fix (move the url printing to the switch statement, rather than using a temporary variable), but the first one would make us lose the benefits of using a structure array to fit the corresponding values.

Comment 1 Marek Polacek 2016-03-22 15:01:39 UTC
I think this behavior is desirable.  I think what has changed is this: given e.g.

void
foo (void)
{
  const char *t = "%d\n";
  __builtin_printf (t, 1);
}

$ xgcc-5 -c g.c -Werror=format=2
says nothing, but with gcc6:
$ xgcc -c g.c -Werror=format=2 
g.c: In function ‘foo’:
g.c:5:3: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
   __builtin_printf (t, 1);
   ^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

Note that with -Wformat=2 both version warn as they should:
g.c: In function ‘foo’:
g.c:5:3: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
   __builtin_printf (t, 1);
   ^

I think this is so since gcc.gnu.org/r231406.

Comment 2 Jakub Jelinek 2016-03-22 15:16:02 UTC
Yeah, handling of -Werror= options which have arguments has been broken in GCC 5 and earlier, so -Werror=format=2 used to work as -Werror=format=1 instead, unless
-Werror=format=2 -Wformat=2 has been used.

Comment 3 Bastien Nocera 2016-03-22 15:21:19 UTC
(In reply to Marek Polacek from comment #1)
> I think this behavior is desirable.

Why is it desirable? In your example, if "t" is a constant, what does this fix? Similarly, something like:
void
foo (void)
{
        const char * const t[] = { 
                "%d\n",
                "%d\n"
        };
        __builtin_printf (t[0], 1); 
}

Also fails, whether t[0] is a constant or not. Are we trying to protect against somebody swapping out the pointer to the string?

Comment 4 Bastien Nocera 2016-03-22 15:21:48 UTC
(In reply to Jakub Jelinek from comment #2)
> Yeah, handling of -Werror= options which have arguments has been broken in
> GCC 5 and earlier, so -Werror=format=2 used to work as -Werror=format=1
> instead, unless
> -Werror=format=2 -Wformat=2 has been used.

Again, what does it fix?

Comment 5 Jakub Jelinek 2016-03-22 15:26:21 UTC
See the documentation?  -Werror=<WHATEVER>=<ARG> is supposed to enable -W<WHATEVER>=<ARG> and make sure that -W<WHATEVER> warnings are reported as errors.
-Wformat (which is the same as -Wformat=1) is some set of format warning options, -Wformat=2 adds other warnings to that.  If you don't want the -Wformat=2 warnings, use just -Werror=format or -Werror=format=1 instead of -Werror=format=2.

Comment 6 Bastien Nocera 2016-03-22 15:32:59 UTC
(In reply to Jakub Jelinek from comment #5)
> See the documentation?  -Werror=<WHATEVER>=<ARG> is supposed to enable
> -W<WHATEVER>=<ARG> and make sure that -W<WHATEVER> warnings are reported as
> errors.
> -Wformat (which is the same as -Wformat=1) is some set of format warning
> options, -Wformat=2 adds other warnings to that.  If you don't want the
> -Wformat=2 warnings, use just -Werror=format or -Werror=format=1 instead of
> -Werror=format=2.

That's not what I'm asking. See comment 3. I'm asking why this ends up being a warning (and then an error) when we're stashing a constant into a variable, and reusing it without changes.

Comment 7 Jakub Jelinek 2016-03-22 15:40:18 UTC
-Wformat is a FE warning, it doesn't look at optimized code, and is not meant to.
The point of the warning is to warn when the FE can't analyze the format string, because it is not simple enough.
In the plugin, you have a structure that contains const char * fields, array of them and index it by some counter.  What if the counter is out of bounds?  In your #c3 testcase, what if somebody in between takes address of the automatic variable?  Even when it is const, somebody (possibly an exploit) could have changed that.  And so on and on.

Comment 8 Jonathan Wakely 2016-03-22 15:47:34 UTC
Or in other words, -Wformat-nonliteral warns about this code by design. That's what it's meant to do. If you don't like that, don't use the warning.

The gcc5 behaviour was a bug, the use of -Werror=format=1 should have enabled -Werror=format-literal, but didn't. That's fixed now, so if you use the warning option you get the warning. If you don't want the warning, don't use the option.

Comment 9 Jonathan Wakely 2016-03-22 15:48:07 UTC
(In reply to Jonathan Wakely from comment #8)
> The gcc5 behaviour was a bug, the use of -Werror=format=1 should have

Oops, typo, I meant -Werror=format=2

Comment 10 Bastien Nocera 2016-03-22 15:50:29 UTC
(In reply to Jakub Jelinek from comment #7)
> -Wformat is a FE warning, it doesn't look at optimized code, and is not
> meant to.
> The point of the warning is to warn when the FE can't analyze the format
> string, because it is not simple enough.
> In the plugin, you have a structure that contains const char * fields, array
> of them and index it by some counter.  What if the counter is out of bounds?
> In your #c3 testcase, what if somebody in between takes address of the
> automatic variable?  Even when it is const, somebody (possibly an exploit)
> could have changed that.  And so on and on.

Right. I know that software is magic, but I wish that the compiler could warn me about the possibly out-of-bounds counter, or not use a temporary variable when unrolling the code, to avoid those problems, instead of me having to refactor code which I can see is correct.

Thanks for the help.

Comment 11 Bastien Nocera 2016-03-22 15:51:18 UTC
(In reply to Jonathan Wakely from comment #8)
> If you don't want the warning, don't
> use the option.

I didn't choose that warning. I'm fairly certain it's one of the default RPM CFLAGS in Fedora.


Note You need to log in before you can comment on or make changes to this bug.