On 3/24/20 7:01 PM, William Lallemand wrote:
On Tue, Mar 24, 2020 at 05:41:48PM +0100, Olivier D wrote:
Hello,
With latest haproxy 2.0, you can generate a simple segfault with only
configuration test (haproxy -f test.cfg -c)
Config content :
----------------------
defaults
mode http
backend test
stick-table type ip size 10k expire 1h store http_req_rate(1h) peers
mypeers
peers mypeers
peer toto 127.0.0.1:1024
-------------------------
Running :
[WARNING] 083/173758 (2599) : Removing incomplete section 'peers mypeers'
(no peer named 'myhostname.localhost').
Segmentation fault
peers_register_table (peers=0xc8a110, table=table@entry=0xc8c6f0) at
src/peers.c:3028
(gdb) bt
#0 peers_register_table (peers=0xc8a110, table=table@entry=0xc8c6f0) at
src/peers.c:3028
#1 0x0000000000522bec in stktable_init (t=0xc8c6f0) at
src/stick_table.c:644
#2 0x000000000050fa67 in check_config_validity () at src/cfgparse.c:4048
#3 0x0000000000505311 in init (argc=<optimized out>, argc@entry=4,
argv=<optimized out>, argv@entry=0x7fffffffe848) at src/haproxy.c:1760
#4 0x000000000046475f in main (argc=4, argv=0x7fffffffe848) at
src/haproxy.c:2727
I know my peer entry is not correct (my hostname is not "toto") but we
should not end with a segfault ^^
I agree. It looks like this is the consequence of the peer being free'd
if you don't reference the local peer in the section. It should still
start in your case by specifying -L toto on the command line.
I think the peer pointer is still referenced in the stick-table which
tries to use it during its initialization. So if we want to free the
peer we need to remove its reference too.
William,
I think you are true, but I did not manage to reproduce this issue. But
I think you are correct about the reference to the peers section which
may be found in stktable structs.
Here is a patch which may fix this issue.
Olivier, please could you give it a try?
Regards.
>From 9b4ea422ceb3a06cb2692d56475dfa18ec02bbf1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Tue, 24 Mar 2020 20:08:30 +0100
Subject: [PATCH] BUG/MINOR: peers: Use after free of "peers" section.
When a "peers" section has not any local peer, it is removed of the list
of "peers" sections by check_config_validity(). But a stick-table which
refers to a "peers" section stores a pointer to this peers section.
These pointer must be reset to NULL value for each stick-table refering to
such a "peers" section to prevent stktable_init() to start the peers frontend
attached to the peers section dereferencing the invalid pointer.
Furthemore this patch stops the peers frontend as this is done for other
configurations invalidated by check_config_validity().
Thank you to Olivier D for having reported this issue with such a
simple configuration file which made haproxy crash when started with
-c option for configuration file validation.
defaults
mode http
peers mypeers
peer toto 127.0.0.1:1024
backend test
stick-table type ip size 10k expire 1h store http_req_rate(1h) peers mypeers
Must be backported to 2.1 and 2.0.
---
src/cfgparse.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/cfgparse.c b/src/cfgparse.c
index eaa853f6e..2c8008312 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -3925,6 +3925,7 @@ out_uri_auth_compat:
*/
last = &cfg_peers;
while (*last) {
+ struct stktable *t;
curpeers = *last;
if (curpeers->state == PR_STSTOPPED) {
@@ -3936,6 +3937,9 @@ out_uri_auth_compat:
else if (!curpeers->peers_fe || !curpeers->peers_fe->id) {
ha_warning("Removing incomplete section 'peers %s' (no peer named '%s').\n",
curpeers->id, localpeer);
+ if (curpeers->peers_fe)
+ stop_proxy(curpeers->peers_fe);
+ curpeers->peers_fe = NULL;
}
else if (atleast2(curpeers->peers_fe->bind_proc)) {
/* either it's totally stopped or too much used */
@@ -3998,6 +4002,11 @@ out_uri_auth_compat:
*/
free(curpeers->id);
curpeers = curpeers->next;
+ /* Reset any refereance to this peers section in the list of stick-tables */
+ for (t = stktables_list; t; t = t->next) {
+ if (t->peers.p && t->peers.p == *last)
+ t->peers.p = NULL;
+ }
free(*last);
*last = curpeers;
}
--
2.20.1
