Skip to content

Commit 1b8c5ab

Browse files
committed
BUG/MAJOR: server: fix stream crash due to deleted server
Before a dynamic server can be deleted, a set of preconditions must be validated to ensure it is not referenced naymore by a stream or a connection. This is implemented in srv_check_for_deletion(). The various criteria specified were incomplete. This allows a server instance to be deleted while still be referenced by a stream and a connection. This bug was reproduced by using ASAN compilation. A script was used to add and delete a server every second, while using h2load to generate traffic with download of 1k objects. Here is the ASAN error. ==140916==ERROR: AddressSanitizer: heap-use-after-free on address 0x520000020080 at pc 0x63cb25679537 bp 0x701529ff5070 sp 0x701529ff5060 READ of size 1 at 0x520000020080 thread T7 #0 0x63cb25679536 in objt_server include/haproxy/obj_type.h:99 #1 0x63cb2568f465 in process_stream src/stream.c:1823 #2 0x63cb25a4a4a2 in run_tasks_from_lists src/task.c:632 #3 0x63cb25a4bf62 in process_runnable_tasks src/task.c:876 #4 0x63cb2596a220 in run_poll_loop src/haproxy.c:3050 #5 0x63cb2596b192 in run_thread_poll_loop src/haproxy.c:3252 #6 0x701539aa9559 (/usr/lib/libc.so.6+0x8b559) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af) #7 0x701539b26a3b (/usr/lib/libc.so.6+0x108a3b) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af) To fix this, add <curr_used_conns> to the counters checked in srv_check_for_deletion(). Outside of this bug, one case which remains sensible is for SF_DIRECT streams which referenced a server instance early in process_stream() before connect_server(). This occurs with use-server directive, force-persist rule or cookie persistence. However, after code reexamination, the code is considered reliable as process_stream() is not rescheduled before connect_server() invocation. These observations have been saved in sess_change_server() documentation to ensure it remains valid in the future. This must be backported up to 2.6.
1 parent f81c00c commit 1b8c5ab

File tree

2 files changed

+15
-1
lines changed

2 files changed

+15
-1
lines changed

src/server.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5796,7 +5796,7 @@ int srv_check_for_deletion(const char *bename, const char *svname, struct proxy
57965796
* TODO idle connections should not prevent server deletion. A proper
57975797
* cleanup function should be implemented to be used here.
57985798
*/
5799-
if (srv->cur_sess || srv->curr_idle_conns ||
5799+
if (srv->curr_used_conns || srv->curr_idle_conns ||
58005800
!eb_is_empty(&srv->queue.head) || srv_has_streams(srv)) {
58015801
msg = "Server still has connections attached to it, cannot remove it.";
58025802
goto leave;

src/stream.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2681,6 +2681,20 @@ void sess_change_server(struct stream *strm, struct server *newsrv)
26812681
{
26822682
struct server *oldsrv = strm->srv_conn;
26832683

2684+
/* Dynamic servers may be deleted during process lifetime. This
2685+
* operation is always conducted under thread isolation. Several
2686+
* conditions prevent deletion, one of them is if server streams list
2687+
* is not empty. sess_change_server() uses stream_add_srv_conn() to
2688+
* ensure the latter condition.
2689+
*
2690+
* A race condition could exist for stream which referenced a server
2691+
* instance (s->target) without registering itself in its server list.
2692+
* This is notably the case for SF_DIRECT streams which referenced a
2693+
* server earlier during process_stream(). However at this time the
2694+
* code is deemed safe as process_stream() cannot be rescheduled before
2695+
* invocation of sess_change_server().
2696+
*/
2697+
26842698
if (oldsrv == newsrv)
26852699
return;
26862700

0 commit comments

Comments
 (0)