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
[ANNOUNCE] haproxy-1.5-dev26 (and hopefully last)
Hi all, So with the completed agent-check updates and the last batch of merged fixes, I think we're ready. I'm emitting dev26 so that everyone can test and report any unexpected annoyance and regressions before we issue -final, and in an attempt to avoid 1.5.1 the same day as 1.5.0. The changes in this version are quite small. Conrad Hoffmann fixed a possible CPU loop when using the systemd wrapper with multiple processes when reloading. Sasha Pachev fixed a possible buffer overflow when using regex which add more than the reserved space to the request or response (very unlikely so low risk but definitely a bug). Thierry Fournier ensured that the str match is used by default for ACLs built from string samples. Olivier Burgard added time-frame filtering to halog. Dirkjan Bussink fixed some minor memory leaks on the error path. Cyril Bonté cleaned up some minor issues on the stats page. And I updated the agent-check to support multiple actions in response, and added a few stats for SSL key generations and SSL cache lookups/misses. Concerning the agent check updates, the agent can now act on these 3 dimensions : - weight (eg: based on CPU load) - service operational status up/down (complementary health checks) - service administrative status (ready/drain/maint) Now it's possible to easily write an agent which only acts on these individual statuses without acting on the other ones if needed. Also all statuses can be forced on the CLI and stats page, and health/agent checks can be stopped. I think that by -final we'll include Rémi's work on making the dhparam size configurable, and Dmitry's proposal to improve the retry/redispatch behaviour to speed up the switching to another server whenever possible. Let's say that without any remaining issue to work on during the upcoming two weeks, we'll release. Feedback welcome as usual, Willy --- Usual links below : Site index : http://haproxy.1wt.eu/ Sources : http://haproxy.1wt.eu/download/1.5/src/devel/ Changelog: http://haproxy.1wt.eu/download/1.5/src/CHANGELOG Cyril's HTML doc : http://cbonte.github.com/haproxy-dconv/configuration-1.5.html And the changelog : 2014/05/28 : 1.5-dev26 - BUG/MEDIUM: polling: fix possible CPU hogging of worker processes after receiving SIGUSR1. - BUG/MINOR: stats: fix a typo on a closing tag for a server tracking another one - OPTIM: stats: avoid the calculation of a useless link on tracking servers in maintenance - MINOR: fix a few memory usage errors - CONTRIB: halog: Filter input lines by date and time through timestamp - MINOR: ssl: SSL_CTX_set_options() and SSL_CTX_set_mode() take a long, not an int - BUG/MEDIUM: regex: fix risk of buffer overrun in exp_replace() - MINOR: acl: set str as default match for strings - DOC: Add some precisions about acl default matching method - MEDIUM: acl: strenghten the option parser to report invalid options - BUG/MEDIUM: config: a stats-less config crashes in 1.5-dev25 - BUG/MINOR: checks: tcp-check must not stop on '\0' for binary checks - MINOR: stats: improve alignment of color codes to save one line of header - MINOR: checks: simplify and improve reporting of state changes when using log-health-checks - MINOR: server: remove the SRV_DRAIN flag which can always be deduced - MINOR: server: use functions to detect state changes and to update them - MINOR: server: create srv_was_usable() from srv_is_usable() and use a pointer - BUG/MINOR: stats: do not report 100% in the thottle column when server is draining - BUG/MAJOR: config: don't free valid regex memory - BUG/MEDIUM: session: don't clear CF_READ_NOEXP if analysers are not called - BUG/MINOR: stats: tracking servers may incorrectly report an inherited DRAIN status - MEDIUM: proxy: make timeout parser a bit stricter - REORG/MEDIUM: server: split server state and flags in two different variables - REORG/MEDIUM: server: move the maintenance bits out of the server state - MAJOR: server: use states instead of flags to store the server state - REORG: checks: put the functions in the appropriate files ! - MEDIUM: server: properly support and propagate the maintenance status - MEDIUM: server: allow multi-level server tracking - CLEANUP: checks: rename the server_status_printf function - MEDIUM: checks: simplify server up/down/nolb transitions - MAJOR: checks: move health checks changes to set_server_check_status() - MINOR: server: make the status reporting function support a reason - MINOR: checks: simplify health check reporting functions - MINOR: server: implement srv_set_stopped() - MINOR: server: implement srv_set_running() - MINOR: server: implement srv_set_stopping() - MEDIUM: checks: simplify failure notification using srv_set_stopped() - MEDIUM: checks: simplify success notification using srv_set_running() - MEDIUM:
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)