On Thu, May 29, 2014 at 12:28:41AM +0200, Vincent Bernat wrote:
> ??? 28 mai 2014 23:16 +0200, Willy Tarreau <[email protected]> :
>
> >> >> src/dumpstats.c:3059:4: error: format not a string literal and no
> >> >> format arguments [-Werror=format-security]
> >> >> chunk_appendf(&trash, srv_hlt_st[1]); /* DOWN (agent) */
> >> >> ^
> >> >>
> >> >> srv_hlt_st[1] is "DOWN %s/%s", so this is not even a false positive. I
> >> >> suppose this should be srv_hlt_st[0] but then it's better to just write
> >> >> "DOWN" (since it avoids the warning).
> >> >
> >> > Huh, no, here it's "DOWN (agent)". We don't even have "%s", the only
> >> > possible
> >> > arguments are %d. Could you please double-check ? Maybe you had local
> >> > changes,
> >> > I don't know, but I'm a bit confused.
> >>
> >> You are right, I was looking at the wrong place in dumpstats.c. So, no
> >> bug, but the compiler is still not happy. What about providing an
> >> additional argument to chunk_appendf to let know that this is handled
> >> correctly?
> >
> > I'm really not fond of adding bugs on purpose to hide compiler bugs,
> > because they tend to be "fixed" by the casual reader the worst possible
> > way... We've had our lot of gcc workarounds already and each time it
> > ended up in a spiral.
> >
> > I just tried here on 4.7 with the same flag and got the same result. I tried
> > to force "const" in addition to "static" on the types declaration and it
> > still
> > fails, so we're clearly in front of a compiler bug. Not a big one, but an
> > invalid check (or an excessive use I don't know). Indeed, there's absolutely
> > nothing wrong about writing :
> >
> > const char *hello = "hello world\n";
> > printf(hello);
> >
> > And when hello is a const, there's no risk that it will be modified at
> > runtime, so basically the check is wrong here if it does not check the
> > real definition of the static element.
> >
> > Do you have an idea how this strange check is dealt with in other
> > programs usually if debian always uses that flag ?
>
> Usually, "printf("%s", blah)" (which is far better than my first
> proposed solution).
Yes but this is for a different thing, it's for when blah is a string
and not a format. Here blah is a format. And it seems that this check
was done precisely to forbid using a variable format, but at the same
time it does not check the format, so it's useless at best, and wrong
at worst.
> "const char * hello" means hello is a pointer to a const char. You may want
> to say "const char * const hello". But gcc doesn't seem to handle it
> either (but clang does).
Yes it does but it doesn't change its verdict. The test is really bogus I
think :
const char fmt[] = "blah"; printf(fmt); => OK
const char *fmt = "blah"; printf(fmt); => KO
const char * const fmt = "blah"; printf(fmt); => KO
const char fmt[][5] = { "blah" }; printf(fmt[0]); => KO
This is the difference between the first one and the last one which makes
me say the test is bogus, because it's exactly the same.
And worst thing is that I guess they added this check for people who
mistakenly use printf(string). And as usual, they don't provide an easy
way to say "don't worry it's not an error, it's on purpose"... This
compiler is becoming more and more irritating, soon we'll have more
lines of workarounds than useful lines of code.
Worse in fact, the workaround is simple, it consists in removing the
__attribute__((printf)) on the declaration line of chunk_appendf(),
and thus *really* opening the door to real scary bugs.
OK so I'll add a dummy argument to shut it up :-(
Willy
