[SCM] CTDB repository - branch 1.0.82 updated - ctdb-1.0.82-65-gd745ecb

[Ronnie Sahlberg]

The branch, 1.0.82 has been updated
       via  d745ecb8d161c7a629676a3b2ab4a2bc826f70b7 (commit)
       via  1eec697a9ff10366b848989b920d6e339cee3976 (commit)
       via  0b6c947d7719d4bf782df0821a18eca4157d9ed5 (commit)
      from  1fc775441e6902e25989268f7395f48bdbc2a5d3 (commit)

http://gitweb.samba.org/?p=sahlberg/ctdb.git;a=shortlog;h=1.0.82


- Log -----------------------------------------------------------------
commit d745ecb8d161c7a629676a3b2ab4a2bc826f70b7
Author: Ronnie Sahlberg <ronniesahlberg at gmail.com>
Date:   Tue Dec 8 11:54:22 2009 +1100

    version 1.0.82-18

commit 1eec697a9ff10366b848989b920d6e339cee3976
Author: Rusty Russell <rusty at rustcorp.com.au>
Date:   Wed Dec 2 08:57:42 2009 +1030

    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 at rustcorp.com.au>

commit 0b6c947d7719d4bf782df0821a18eca4157d9ed5
Author: Ronnie Sahlberg <ronniesahlberg at gmail.com>
Date:   Tue Dec 8 11:52:20 2009 +1100

    Revert "prevent doubly freeing memory on invalid packet"
    
    This reverts commit 1975b53b5ea608d60b8b3e527a435a7c817a97ea.

-----------------------------------------------------------------------

Summary of changes:
 common/ctdb_io.c        |   20 +++++++++++++++++++-
 packaging/RPM/ctdb.spec |    4 +++-
 tcp/tcp_io.c            |   10 +---------
 3 files changed, 23 insertions(+), 11 deletions(-)


Changeset truncated at 500 lines:

diff --git a/common/ctdb_io.c b/common/ctdb_io.c
index 3377aa1..67d8b6c 100644
--- 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;
 }
diff --git a/packaging/RPM/ctdb.spec b/packaging/RPM/ctdb.spec
index 534e128..17e646e 100644
--- a/packaging/RPM/ctdb.spec
+++ b/packaging/RPM/ctdb.spec
@@ -5,7 +5,7 @@ Vendor: Samba Team
 Packager: Samba Team <samba at samba.org>
 Name: ctdb
 Version: 1.0.82
-Release: 16
+Release: 18
 Epoch: 0
 License: GNU GPL version 3
 Group: System Environment/Daemons
@@ -133,6 +133,8 @@ fi
 %{_libdir}/pkgconfig/ctdb.pc
 
 %changelog
+* Tue Dec 8 2009 : Version 1.0.82-18
+ - Remove dodgy talloc fix and replace with correct fix from Rusty.
 * Fri Nov 6 2009 : Version 1.0.82-16
  - a tallock free bug in tcp transport   fixed by wolfgang mueller
  - make sure to ban nodes quuickly if they disagree on the flags of the nodes in teh cluster
diff --git a/tcp/tcp_io.c b/tcp/tcp_io.c
index 257446c..10711d1 100644
--- a/tcp/tcp_io.c
+++ b/tcp/tcp_io.c
@@ -75,15 +75,7 @@ void ctdb_tcp_read_cb(uint8_t *data, size_t cnt, void *args)
 	return;
 
 failed:
-	/*
-	 * removed the talloc_free(in) in order to prevent a double free on 'in'
-	 * Questions: 
-	 *  - Are we introducing a memory leak? I don't think so.
-	 *  - Is there a destructor missing that removes this block from the queue
-	 *    it was already added? Yes, I think this is missing.
-	 *  - Why do we get such invalid packages? Hmm....
-	 */
-	return;
+	talloc_free(in);
 }
 
 /*


-- 
CTDB repository