Attachment 6743 Details for Bug 8319 – Patch to make queue_io_read() safe for reentry - rb2

[patch] Patch to make queue_io_read() safe for reentry - rb2

make-queue_io_read-safe-for-reentry-rb2.patch (text/plain), 5.60 KB, created by David Disseldorp on 2011-08-01 15:43:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Disseldorp

Created: 2011-08-01 15:43:13 UTC

Size: 5.60 KB

patch

obsolete

>From 3ac7ec262e8756e51447b5979d3bbc0f198cdc0f Mon Sep 17 00:00:00 2001
>From: David Disseldorp <ddiss@suse.de>
>Date: Sun, 31 Jul 2011 03:14:54 +0200
>Subject: [PATCH] io: Make queue_io_read() safe for reentry
>
>queue_io_read() may be reentered via the queue callback, recoverd is
>particularly guilty of this.
>
>queue_io_read() is not safe for reentry if more than one packet is
>received and partial chunks follow - data read off the pipe on re-entry
>is assumed to be the start-of-packet four byte length. This leads to a
>wrongly aligned stream and the notorious "Invalid packet of length 0"
>errors.
>
>This change fixes queue_io_read() to be safe under reentry, only a
>single packet is processed per call.
>
>https://bugzilla.samba.org/show_bug.cgi?id=8319
>---
> common/ctdb_io.c |  129 ++++++++++++++++++++++++++----------------------------
> 1 files changed, 62 insertions(+), 67 deletions(-)
>
>diff --git a/common/ctdb_io.c b/common/ctdb_io.c
>index b7feed9..f791be1 100644
>--- a/common/ctdb_io.c
>+++ b/common/ctdb_io.c
>@@ -64,12 +64,17 @@ int ctdb_queue_length(struct ctdb_queue *queue)
> 
> /*
>   called when an incoming connection is readable
>+  This function MUST be safe for reentry via the queue callback!
> */
> static void queue_io_read(struct ctdb_queue *queue)
> {
> 	int num_ready = 0;
>+	uint32_t sz_bytes_req;
>+	uint32_t pkt_size;
>+	uint32_t pkt_bytes_remaining;
>+	uint32_t to_read;
> 	ssize_t nread;
>-	uint8_t *data, *data_base;
>+	uint8_t *data;
> 
> 	if (ioctl(queue->fd, FIONREAD, &num_ready) != 0) {
> 		return;
>@@ -79,88 +84,78 @@ static void queue_io_read(struct ctdb_queue *queue)
> 		goto failed;
> 	}
> 
>-
>-	queue->partial.data = talloc_realloc_size(queue, queue->partial.data, 
>-						  num_ready + queue->partial.length);
>-
> 	if (queue->partial.data == NULL) {
>-		DEBUG(DEBUG_ERR,("read error alloc failed for %u\n", 
>-			 num_ready + queue->partial.length));
>-		goto failed;
>-	}
>-
>-	nread = read(queue->fd, queue->partial.data + queue->partial.length, num_ready);
>-	if (nread <= 0) {
>-		DEBUG(DEBUG_ERR,("read error nread=%d\n", (int)nread));
>-		goto failed;
>+		/* starting fresh, allocate buf for size bytes */
>+		sz_bytes_req = sizeof(pkt_size);
>+		queue->partial.data = talloc_size(queue, sz_bytes_req);
>+		if (queue->partial.data == NULL) {
>+			DEBUG(DEBUG_ERR,("read error alloc failed for %u\n",
>+					 sz_bytes_req));
>+			goto failed;
>+		}
>+	} else if (queue->partial.length < sizeof(pkt_size)) {
>+		/* yet to find out the packet length */
>+		sz_bytes_req = sizeof(pkt_size) - queue->partial.length;
>+	} else {
>+		/* partial packet, length known, full buf allocated */
>+		sz_bytes_req = 0;
> 	}
>-
>-
> 	data = queue->partial.data;
>-	nread += queue->partial.length;
>-
>-	queue->partial.data = NULL;
>-	queue->partial.length = 0;
>-
>-	if (nread >= 4 && *(uint32_t *)data == nread) {
>-		/* it is the responsibility of the incoming packet
>-		 function to free 'data' */
>-		queue->callback(data, nread, queue->private_data);
>-		return;
>-	}
>-
>-	data_base = data;
>-
>-	while (nread >= 4 && *(uint32_t *)data <= nread) {
>-		/* 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 */
>-			DEBUG(DEBUG_CRIT,("Invalid packet of length 0\n"));
>+	if (sz_bytes_req > 0) {
>+		to_read = MIN(sz_bytes_req, num_ready);
>+		nread = read(queue->fd, data + queue->partial.length, to_read);
>+		if (nread <= 0) {
>+			DEBUG(DEBUG_ERR,("read error nread=%d\n", (int)nread));
> 			goto failed;
> 		}
>-		d2 = talloc_memdup(queue, data, len);
>-		if (d2 == NULL) {
>-			DEBUG(DEBUG_ERR,("read error memdup failed for %u\n", len));
>-			/* sigh */
>+		queue->partial.length += nread;
>+
>+		if (nread < sz_bytes_req) {
>+			/* not enough to know the length */
>+			DEBUG(DEBUG_DEBUG,("Partial packet length read\n"));
>+			return;
>+		}
>+		/* size now known, allocate buffer for the full packet */
>+		queue->partial.data = talloc_realloc_size(queue, data,
>+							  *(uint32_t *)data);
>+		if (queue->partial.data == NULL) {
>+			DEBUG(DEBUG_ERR,("read error alloc failed for %u\n",
>+					 *(uint32_t *)data));
> 			goto failed;
> 		}
>-
>-		queue->destroyed = &destroyed;
>-		queue->callback(d2, len, queue->private_data);
>-		/* If callback freed us, don't do anything else. */
>-		if (destroyed) {
>+		data = queue->partial.data;
>+		num_ready -= nread;
>+		if (num_ready == 0) {
>+			DEBUG(DEBUG_DEBUG,("Got pkt length bytes only\n"));
> 			return;
> 		}
>-		queue->destroyed = NULL;
>+	}
> 
>-		data += len;
>-		nread -= len;		
>+	pkt_size = *(uint32_t *)data;
>+	if (pkt_size == 0) {
>+		DEBUG(DEBUG_CRIT,("Invalid packet of length 0\n"));
>+		goto failed;
> 	}
> 
>-	if (nread > 0) {
>-		/* we have only part of a packet */
>-		if (data_base == data) {
>-			queue->partial.data = data;
>-			queue->partial.length = nread;
>-		} else {
>-			queue->partial.data = talloc_memdup(queue, data, nread);
>-			if (queue->partial.data == NULL) {
>-				DEBUG(DEBUG_ERR,("read error memdup partial failed for %u\n", 
>-					 (unsigned)nread));
>-				goto failed;
>-			}
>-			queue->partial.length = nread;
>-			talloc_free(data_base);
>-		}
>+	pkt_bytes_remaining = pkt_size - queue->partial.length;
>+	to_read = MIN(pkt_bytes_remaining, num_ready);
>+	nread = read(queue->fd, data + queue->partial.length, to_read);
>+	if (nread <= 0) {
>+		DEBUG(DEBUG_ERR,("read error nread=%d\n", (int)nread));
>+		goto failed;
>+	}
>+
>+	queue->partial.length += nread;
>+	if (queue->partial.length < pkt_size) {
>+		DEBUG(DEBUG_DEBUG,("Partial packet data read\n"));
> 		return;
> 	}
> 
>-	talloc_free(data_base);
>+	queue->partial.data = NULL;
>+	queue->partial.length = 0;
>+	/* it is the responsibility of the callback to free 'data' */
>+	queue->callback(data, pkt_size, queue->private_data);
> 	return;
> 
> failed:
>-- 
>1.7.1
>

Actions: View

Attachments on bug 8319: 6715 | 6737 | 6738 | 6742 | 6743