From: Willy Tarreau <w@1wt.eu>
Date: Sun, 22 Feb 2009 14:17:24 +0000 (+0100)
Subject: [BUG] fix random memory corruption using "show sess"
X-Git-Tag: v1.3.16-rc1~42
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fd3828e263bce5e97c4e1f31aebc380addb6a897;p=thirdparty%2Fhaproxy.git

[BUG] fix random memory corruption using "show sess"

Commit 8a5c626e73bac905d150185e45110525588d7b4c introduced the sessions
dump on the unix socket. This implementation is buggy because it may try
to link to the sessions list's head after the last session is removed
with a backref. Also, for the LIST_ISEMPTY test to succeed, we have to
proceed with LIST_INIT after LIST_DEL.
---

diff --git a/src/client.c b/src/client.c
index bf8519f177..4cc3d7ea03 100644
--- a/src/client.c
+++ b/src/client.c
@@ -66,7 +66,6 @@ int event_accept(int fd) {
 	struct session *s;
 	struct http_txn *txn;
 	struct task *t;
-	struct bref *bref, *back;
 	int cfd;
 	int max_accept = global.tune.maxaccept;
 
@@ -470,11 +469,6 @@ int event_accept(int fd) {
  out_free_task:
 	pool_free2(pool2_task, t);
  out_free_session:
-	list_for_each_entry_safe(bref, back, &s->back_refs, users) {
-		LIST_DEL(&bref->users);
-		LIST_ADDQ(&LIST_ELEM(s->list.n, struct session *, list)->back_refs, &bref->users);
-		bref->ref = s->list.n;
-	}
 	LIST_DEL(&s->list);
 	pool_free2(pool2_session, s);
  out_close:
diff --git a/src/dumpstats.c b/src/dumpstats.c
index 07e505649d..b12fbd0b1e 100644
--- a/src/dumpstats.c
+++ b/src/dumpstats.c
@@ -1099,8 +1099,10 @@ void stats_dump_sess_to_buffer(struct session *s, struct buffer *rep)
 		 * reference to the last session being dumped.
 		 */
 		if (s->data_state == DATA_ST_LIST) {
-			if (!LIST_ISEMPTY(&s->data_ctx.sess.bref.users))
+			if (!LIST_ISEMPTY(&s->data_ctx.sess.bref.users)) {
 				LIST_DEL(&s->data_ctx.sess.bref.users);
+				LIST_INIT(&s->data_ctx.sess.bref.users);
+			}
 		}
 		s->data_state = DATA_ST_FIN;
 		buffer_stop_hijack(rep);
@@ -1118,7 +1120,10 @@ void stats_dump_sess_to_buffer(struct session *s, struct buffer *rep)
 	case DATA_ST_INIT:
 		/* the function had not been called yet, let's prepare the
 		 * buffer for a response. We initialize the current session
-		 * pointer to the first in the global list.
+		 * pointer to the first in the global list. When a target
+		 * session is being destroyed, it is responsible for updating
+		 * this pointer. We know we have reached the end when this
+		 * pointer points back to the head of the sessions list.
 		 */
 		stream_int_retnclose(rep->cons, &msg);
 		LIST_INIT(&s->data_ctx.sess.bref.users);
@@ -1127,16 +1132,19 @@ void stats_dump_sess_to_buffer(struct session *s, struct buffer *rep)
 		/* fall through */
 
 	case DATA_ST_LIST:
+		/* first, let's detach the back-ref from a possible previous session */
+		if (!LIST_ISEMPTY(&s->data_ctx.sess.bref.users)) {
+			LIST_DEL(&s->data_ctx.sess.bref.users);
+			LIST_INIT(&s->data_ctx.sess.bref.users);
+		}
+
+		/* and start from where we stopped */
 		while (s->data_ctx.sess.bref.ref != &sessions) {
 			char pn[INET6_ADDRSTRLEN + strlen(":65535")];
 			struct session *curr_sess;
 
 			curr_sess = LIST_ELEM(s->data_ctx.sess.bref.ref, struct session *, list);
 
-			/* first, let's detach the back-ref from a possible previous session */
-			if (!LIST_ISEMPTY(&s->data_ctx.sess.bref.users))
-				LIST_DEL(&s->data_ctx.sess.bref.users);
-
 			chunk_printf(&msg, sizeof(trash),
 				     "%p: proto=%s",
 				     curr_sess,
@@ -1194,7 +1202,9 @@ void stats_dump_sess_to_buffer(struct session *s, struct buffer *rep)
 					     : "never");
 
 			if (buffer_write_chunk(rep, &msg) >= 0) {
-				/* let's try again later */
+				/* let's try again later from this session. We add ourselves into
+				 * this session's users so that it can remove us upon termination.
+				 */
 				LIST_ADDQ(&curr_sess->back_refs, &s->data_ctx.sess.bref.users);
 				return;
 			}
diff --git a/src/proto_uxst.c b/src/proto_uxst.c
index f07fa158bb..997cf496eb 100644
--- a/src/proto_uxst.c
+++ b/src/proto_uxst.c
@@ -360,7 +360,6 @@ int uxst_event_accept(int fd) {
 	struct listener *l = fdtab[fd].owner;
 	struct session *s;
 	struct task *t;
-	struct bref *bref, *back;
 	int cfd;
 	int max_accept;
 
@@ -553,11 +552,6 @@ int uxst_event_accept(int fd) {
  out_free_task:
 	pool_free2(pool2_task, t);
  out_free_session:
-	list_for_each_entry_safe(bref, back, &s->back_refs, users) {
-		LIST_DEL(&bref->users);
-		LIST_ADDQ(&LIST_ELEM(s->list.n, struct session *, list)->back_refs, &bref->users);
-		bref->ref = s->list.n;
-	}
 	LIST_DEL(&s->list);
 	pool_free2(pool2_session, s);
  out_close:
diff --git a/src/session.c b/src/session.c
index d1bc052aa3..8ced9b8bf3 100644
--- a/src/session.c
+++ b/src/session.c
@@ -98,8 +98,13 @@ void session_free(struct session *s)
 	pool_free2(pool2_capture, txn->srv_cookie);
 
 	list_for_each_entry_safe(bref, back, &s->back_refs, users) {
+		/* we have to unlink all watchers. We must not relink them if
+		 * this session was the last one in the list.
+		 */
 		LIST_DEL(&bref->users);
-		LIST_ADDQ(&LIST_ELEM(s->list.n, struct session *, list)->back_refs, &bref->users);
+		LIST_INIT(&bref->users);
+		if (s->list.n != &sessions)
+			LIST_ADDQ(&LIST_ELEM(s->list.n, struct session *, list)->back_refs, &bref->users);
 		bref->ref = s->list.n;
 	}
 	LIST_DEL(&s->list);