From 3ac7ec262e8756e51447b5979d3bbc0f198cdc0f Mon Sep 17 00:00:00 2001 From: David Disseldorp 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