PATCH: ctdb: buffer write beyond limits

[swen]

Hi Volker

thanks for your review.

On Mon, 2019-02-18 at 17:49 +0100, Volker Lendecke wrote:
> On Mon, Feb 18, 2019 at 08:19:52AM +0100, swen via samba-technical
> wrote:
> >  data_read:
> > -	num_ready = MIN(num_ready, queue->buffer.size - queue-
> > >buffer.length);
> > +	num_ready = MIN(num_ready,
> > +			queue->buffer.size -
> > +				(queue->buffer.length + queue-
> > >buffer.offset));
> 
> Whenever I see buffer calculations without explicit overflow checks
> I'm scared. So either add a very explicit comment explaining in 100%
> simple words that the addition can never overflow and the subtraction
> can never underflow or add the appropriate over/undeflow checks.
> 
Please have a look at the updated patch with extensive comments.
I hope that explanation is sufficient.

Cheers Swen
-------------- next part --------------
From bd2fc4c6b972cf0483801359d7cc11e3828d95ce Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Fri, 15 Feb 2019 14:34:05 +0100
Subject: [PATCH] ctdb: buffer write beyond limits

In order to calculate the number of bytes correctly which
are to be read into the buffer, the buffer.offset must be taken
into account.

This patch fixes a regression introduced by 382705f495dd.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13791

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
---
 ctdb/common/ctdb_io.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/ctdb/common/ctdb_io.c b/ctdb/common/ctdb_io.c
index d86540762ea..c565619526a 100644
--- a/ctdb/common/ctdb_io.c
+++ b/ctdb/common/ctdb_io.c
@@ -226,7 +226,21 @@ buffer_shift:
 	}
 
 data_read:
-	num_ready = MIN(num_ready, queue->buffer.size - queue->buffer.length);
+	/**********************************************************************
+	 * why this works: the sum of buffer.length and buffer.offset can never
+	 * be bigger than buffer.size because the number or read bytes
+	 * can max be equal to buffer.size and only if buffer.offset is zero
+	 * which is the initial value. This number of read bytes will become
+	 * the buffer.length value. From this point on buffer.offset can
+	 * only grow as much as buffer.length is shrinking. See queue_process()
+	 * In adition the provided (from ioctl) value for the number of
+	 * available bytes to read (num_ready) is casted to uint32_t which
+	 * would turn a negative value into a positive asuring MIN() is 
+	 * returning the smallest positive value of the two.
+	 * *******************************************************************/
+	num_ready = MIN((uint32_t)num_ready,
+			queue->buffer.size -
+				(queue->buffer.length + queue->buffer.offset));
 
 	if (num_ready > 0) {
 		nread = sys_read(queue->fd,
-- 
2.20.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190218/e3b6bbcd/signature.sig>