ctdb_io: fix use-after-free on invalid packets

Wolfgang saw a talloc complaint about using freed memory in ctdb_tcp_read_cb.
His fix was to remove the talloc_free() in that function, which causes
loops when a socket is closed (as it does not get removed from the event
system), eg:
	netcat 192.168.1.2 4379 < /dev/null

The real bug is that when we have more than one pending packet in the
queue, we loop calling the callback without any safeguards should that
callback free the queue (as it tends to do on invalid packets).  This
can be reproduced by sending more than one bogus packet at once:
	# Length word at start: 4 == empty packet (assumed little endian)
	/usr/bin/printf \\4\\0\\0\\0\\4\\0\\0\\0 > /tmp/pkt
	netcat 192.168.1.2 4379 < /tmp/pkt

Using a destructor we can check if the callback frees us, and exit
immediately.  Elsewhere, we return after the callback anyway.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/common/ctdb_io.c b/common/ctdb_io.c
index 3377aa13338b25b9674f846ea5081ce3e9de1544..67d8b6cd411210166f5c10d84deeb4bbcd2dea61 100644 (file)
--- a/common/ctdb_io.c
+++ b/common/ctdb_io.c
@@ -51,6 +51,7 @@ struct ctdb_queue {
        size_t alignment;
        void *private_data;
        ctdb_queue_cb_fn_t callback;
+       bool *destroyed;
 };
 
 
@@ -108,6 +109,8 @@ static void queue_io_read(struct ctdb_queue *queue)
                /* we have at least one packet */
                uint8_t *d2;
                uint32_t len;
+               bool destroyed = false;
+
                len = *(uint32_t *)data;
                if (len == 0) {
                        /* bad packet! treat as EOF */
@@ -120,7 +123,15 @@ static void queue_io_read(struct ctdb_queue *queue)
                        /* sigh */
                        goto failed;
                }
+
+               queue->destroyed = &destroyed;
                queue->callback(d2, len, queue->private_data);
+               /* If callback freed us, don't do anything else. */
+               if (destroyed) {
+                       return;
+               }
+               queue->destroyed = NULL;
+
                data += len;
                nread -= len;           
        }
@@ -328,7 +339,13 @@ int ctdb_queue_set_fd(struct ctdb_queue *queue, int fd)
        return 0;
 }
 
-
+/* If someone sets up this pointer, they want to know if the queue is freed */
+static int queue_destructor(struct ctdb_queue *queue)
+{
+       if (queue->destroyed != NULL)
+               *queue->destroyed = true;
+       return 0;
+}
 
 /*
   setup a packet queue on a socket
@@ -355,6 +372,7 @@ struct ctdb_queue *ctdb_queue_setup(struct ctdb_context *ctdb,
                        return NULL;
                }
        }
+       talloc_set_destructor(queue, queue_destructor);
 
        return queue;
 }