[tabled patch 1/1] running completions over disposed cli

[tabled patch 1/1] running completions over disposed cli

[Pete Zaitcev]

Miracluously this never actually crashed on me, but I added unrelated
debugging printout into the dispatch routine and it printed weird
values. Then it dawned on me that a state change function may dispose
of the struct cli, in which case cli_write_run_compl is use-after-free.

It may seem that checking if the old state was evt_dispose before
running cli_write_run_compl is an expedient fix, but that does not
work, because we do not always dispose of the cli in such case.
If the cli to be disposed still has anything in the queue, we
need to continue to deliver events, and for that we have to
run outstanding completions.

So, we go a longer route and re-hook the list of completions
to a per-server global instead of a client. The patch is straight-
forward. The only thing we need to be careful is to make sure
that no outstanding completions are left in the queue before
freeing a client struct. This is ensured by force-running completions.

One other necessary change was to add a back poiter from a completion
to the current client. This is because one caller needed the client
pointer (object_get_more).

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

---
 server/object.c |    2 +-
 server/server.c |   29 ++++++++++++++---------------
 server/tabled.h |    6 +++---
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/server/object.c b/server/object.c
--- a/server/object.c
+++ b/server/object.c
@@ -1056,7 +1056,7 @@ static bool object_get_more(struct client *cli, void *cb_data, bool done)
 static void object_get_event(struct open_chunk *ochunk)
 {
 	object_get_poke(ochunk->cli);
-	cli_write_run_compl(ochunk->cli);
+	cli_write_run_compl();
 }
 
 static bool object_get_body(struct client *cli, const char *user,
diff --git a/server/server.c b/server/server.c
index 8d100f7..1c7114a 100644
--- a/server/server.c
+++ b/server/server.c
@@ -437,12 +437,12 @@ bool stat_status(struct client *cli, GList *content)
 static void cli_write_complete(struct client *cli, struct client_write *tmp)
 {
 	list_del(&tmp->node);
-	list_add_tail(&tmp->node, &cli->write_compl_q);
+	list_add_tail(&tmp->node, &tabled_srv.write_compl_q);
 }
 
-static bool cli_write_free(struct client *cli, struct client_write *tmp,
-			   bool done)
+static bool cli_write_free(struct client_write *tmp, bool done)
 {
+	struct client *cli = tmp->cb_cli;
 	bool rcb = false;
 
 	cli->write_cnt -= tmp->length;
@@ -458,24 +458,22 @@ static void cli_write_free_all(struct client *cli)
 {
 	struct client_write *wr, *tmp;
 
-	list_for_each_entry_safe(wr, tmp, &cli->write_compl_q, node) {
-		cli_write_free(cli, wr, true);
-	}
+	cli_write_run_compl();
 	list_for_each_entry_safe(wr, tmp, &cli->write_q, node) {
-		cli_write_free(cli, wr, false);
+		cli_write_free(wr, false);
 	}
 }
 
-bool cli_write_run_compl(struct client *cli)
+bool cli_write_run_compl(void)
 {
 	struct client_write *wr;
 	bool do_loop;
 
 	do_loop = false;
-	while (!list_empty(&cli->write_compl_q)) {
-		wr = list_entry(cli->write_compl_q.next, struct client_write,
-				node);
-		do_loop |= cli_write_free(cli, wr, true);
+	while (!list_empty(&tabled_srv.write_compl_q)) {
+		wr = list_entry(tabled_srv.write_compl_q.next,
+				struct client_write, node);
+		do_loop |= cli_write_free(wr, true);
 	}
 	return do_loop;
 }
@@ -658,6 +656,7 @@ int cli_writeq(struct client *cli, const void *buf, unsigned int buflen,
 	wr->length = buflen;
 	wr->cb = cb;
 	wr->cb_data = cb_data;
+	wr->cb_cli = cli;
 	list_add_tail(&wr->node, &cli->write_q);
 	cli->write_cnt += buflen;
 	if (cli->write_cnt > cli->write_cnt_max)
@@ -1323,7 +1322,6 @@ static struct client *cli_alloc(bool is_status)
 	cli->state = evt_read_req;
 	cli->evt_table = is_status? evt_funcs_status: evt_funcs_server;
 	INIT_LIST_HEAD(&cli->write_q);
-	INIT_LIST_HEAD(&cli->write_compl_q);
 	INIT_LIST_HEAD(&cli->out_ch);
 	cli->req_ptr = cli->req_buf;
 	memset(&cli->req, 0, sizeof(cli->req) - sizeof(cli->req.hdr));
@@ -1336,7 +1334,7 @@ static void tcp_cli_wr_event(int fd, short events, void *userdata)
 	struct client *cli = userdata;
 
 	cli_writable(cli);
-	cli_write_run_compl(cli);
+	cli_write_run_compl();
 }
 
 static void tcp_cli_event(int fd, short events, void *userdata)
@@ -1346,7 +1344,7 @@ static void tcp_cli_event(int fd, short events, void *userdata)
 
 	do {
 		loop = cli->evt_table[cli->state](cli, events);
-		loop |= cli_write_run_compl(cli);
+		loop |= cli_write_run_compl();
 	} while (loop);
 }
 
@@ -1876,6 +1874,7 @@ int main (int argc, char *argv[])
 	struct event_base *event_base_rep;
 
 	INIT_LIST_HEAD(&tabled_srv.all_stor);
+	INIT_LIST_HEAD(&tabled_srv.write_compl_q);
 	tabled_srv.state_tdb = ST_TDB_INIT;
 
 	/* isspace() and strcasecmp() consistency requires this */
diff --git a/server/tabled.h b/server/tabled.h
index 75fa147..b6e4cbb 100644
--- a/server/tabled.h
+++ b/server/tabled.h
@@ -69,7 +69,6 @@ enum errcode {
 };
 
 struct client;
-struct client_write;
 
 enum {
 	pat_auth,
@@ -111,6 +110,7 @@ struct client_write {
 	int			length;		/* length for accounting */
 	cli_write_func		cb;		/* callback */
 	void			*cb_data;	/* data passed to cb */
+	struct client		*cb_cli;	/* cli passed to cb */
 
 	struct list_head	node;
 };
@@ -164,7 +164,6 @@ struct client {
 	struct event		write_ev;
 
 	struct list_head	write_q;	/* list of async writes */
-	struct list_head	write_compl_q;	/* list of done writes */
 	size_t			write_cnt;	/* water level */
 	bool			writing;
 	/* some debugging stats */
@@ -233,6 +232,7 @@ struct server {
 	struct event_base	*evbase_main;
 	int			ev_pipe[2];
 	struct event		pevt;
+	struct list_head	write_compl_q;	/* list of done writes */
 
 	char			*config;	/* config file (static) */
 
@@ -329,7 +329,7 @@ extern int cli_writeq(struct client *cli, const void *buf, unsigned int buflen,
 extern size_t cli_wqueued(struct client *cli);
 extern bool cli_cb_free(struct client *cli, void *cb_data, bool done);
 extern bool cli_write_start(struct client *cli);
-extern bool cli_write_run_compl(struct client *cli);
+extern bool cli_write_run_compl(void);
 extern int cli_req_avail(struct client *cli);
 extern void applog(int prio, const char *fmt, ...);
 extern int stor_update_cb(void);

Re: [tabled patch 1/1] running completions over disposed cli

[Jeff Garzik]

On 05/15/2010 03:17 PM, Pete Zaitcev wrote:
> Miracluously this never actually crashed on me, but I added unrelated
> debugging printout into the dispatch routine and it printed weird
> values. Then it dawned on me that a state change function may dispose
> of the struct cli, in which case cli_write_run_compl is use-after-free.
>
> It may seem that checking if the old state was evt_dispose before
> running cli_write_run_compl is an expedient fix, but that does not
> work, because we do not always dispose of the cli in such case.
> If the cli to be disposed still has anything in the queue, we
> need to continue to deliver events, and for that we have to
> run outstanding completions.
>
> So, we go a longer route and re-hook the list of completions
> to a per-server global instead of a client. The patch is straight-
> forward. The only thing we need to be careful is to make sure
> that no outstanding completions are left in the queue before
> freeing a client struct. This is ensured by force-running completions.
>
> One other necessary change was to add a back poiter from a completion
> to the current client. This is because one caller needed the client
> pointer (object_get_more).
>
> Signed-off-by: Pete Zaitcev<zaitcev@redhat.com>

applied