Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
On 29 мая 2014 г., at 3:04, Willy Tarreau w...@1wt.eu wrote:
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 :-(
Just for reference: clang also warns here:
cc -Iinclude -Iebtree -Wall -O2 -pipe -fno-strict-aliasing -DFREEBSD_PORTS
-DTPROXY -DCONFIG_HAP_CRYPT -DUSE_GETADDRINFO -DUSE_ZLIB -DENABLE_POLL
-DENABLE_KQUEUE -DUSE_OPENSSL -DCONFIG_HAPROXY_VERSION=\1.5-dev26-2e85840\
-DCONFIG_HAPROXY_DATE=\2014/05/28\ -c -o src/dumpstats.o src/dumpstats.c
src/dumpstats.c:3059:26: warning: format string is not a string literal
(potentially insecure) [-Wformat-security]
chunk_appendf(trash, srv_hlt_st[1]); /* DOWN (agent) */
^
FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
Target: x86_64-unknown-freebsd10.0
Thread model: posix
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
❦ 28 mai 2014 18:11 +0200, Willy Tarreau w...@1wt.eu : Feedback welcome as usual, When compiling with -Werror=format-security (which is a common settings on a Debian-based distribution), we get: 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). It leads me to the next chunk of code: chunk_appendf(trash, srv_hlt_st[state], (ref-state != SRV_ST_STOPPED) ? (ref-check.health - ref-check.rise + 1) : (ref-check.health), (ref-state != SRV_ST_STOPPED) ? (ref-check.fall) : (ref-check.rise)); Not all members of srv_hlt_st have %s/%s. I cannot say for sure how chunk_appendf work. Is that the caller or the callee that clean up? I suppose that because of ..., this is automatically the caller so the additional arguments are harmless. -- panic(esp: what could it be... I wonder...); 2.2.16 /usr/src/linux/drivers/scsi/esp.c
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
On Wed, May 28, 2014 at 08:43:10PM +0200, Vincent Bernat wrote: ❦ 28 mai 2014 18:11 +0200, Willy Tarreau w...@1wt.eu : Feedback welcome as usual, When compiling with -Werror=format-security (which is a common settings on a Debian-based distribution), we get: 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) */ ^ I'm getting the same error when building against Fedora rawhide. Ryan 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). It leads me to the next chunk of code: chunk_appendf(trash, srv_hlt_st[state], (ref-state != SRV_ST_STOPPED) ? (ref-check.health - ref-check.rise + 1) : (ref-check.health), (ref-state != SRV_ST_STOPPED) ? (ref-check.fall) : (ref-check.rise)); Not all members of srv_hlt_st have %s/%s. I cannot say for sure how chunk_appendf work. Is that the caller or the callee that clean up? I suppose that because of ..., this is automatically the caller so the additional arguments are harmless. -- panic(esp: what could it be... I wonder...); 2.2.16 /usr/src/linux/drivers/scsi/esp.c
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
Hi Vincent, On Wed, May 28, 2014 at 08:43:10PM +0200, Vincent Bernat wrote: ??? 28 mai 2014 18:11 +0200, Willy Tarreau w...@1wt.eu : Feedback welcome as usual, When compiling with -Werror=format-security (which is a common settings on a Debian-based distribution), we get: 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. It leads me to the next chunk of code: chunk_appendf(trash, srv_hlt_st[state], (ref-state != SRV_ST_STOPPED) ? (ref-check.health - ref-check.rise + 1) : (ref-check.health), (ref-state != SRV_ST_STOPPED) ? (ref-check.fall) : (ref-check.rise)); Not all members of srv_hlt_st have %s/%s. I cannot say for sure how chunk_appendf work. Is that the caller or the callee that clean up? I suppose that because of ..., this is automatically the caller so the additional arguments are harmless. They're %d/%d, not %s/%s. The extra args are ignored when not used by the format string, just like printf does. In fact, chunk_appendf() does nothing special, it just uses vsnprintf(), which itself only parses the format arguments and depile them from the stack when needed. Hoping this helps, Willy
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
❦ 28 mai 2014 22:59 +0200, Willy Tarreau w...@1wt.eu : When compiling with -Werror=format-security (which is a common settings on a Debian-based distribution), we get: 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? -- /* Identify the flock of penguins. */ 2.2.16 /usr/src/linux/arch/alpha/kernel/setup.c
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
On Wed, May 28, 2014 at 11:04:45PM +0200, Vincent Bernat wrote: ??? 28 mai 2014 22:59 +0200, Willy Tarreau w...@1wt.eu : When compiling with -Werror=format-security (which is a common settings on a Debian-based distribution), we get: 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 ? Willy
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
❦ 28 mai 2014 23:16 +0200, Willy Tarreau w...@1wt.eu : 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). 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). -- panic(sun_82072_fd_inb: How did I get here?); 2.2.16 /usr/src/linux/include/asm-sparc/floppy.h
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
On Thu, May 29, 2014 at 12:28:41AM +0200, Vincent Bernat wrote:
??? 28 mai 2014 23:16 +0200, Willy Tarreau w...@1wt.eu :
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
Re: [ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
❦ 29 mai 2014 01:04 +0200, Willy Tarreau w...@1wt.eu :
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.
Well, this is something which exists since a long time. At least 5 years.
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 :-(
Or you could declare unsafe_chunk_appendf without the attribute and make
chunk_appendf call this one and hope the compiler will optimize the
indirection. But that's quite complex when you only need to add a dummy
argument.
--
Each module should do one thing well.
- The Elements of Programming Style (Kernighan Plauger)
