PATCH: ctdb: buffer write beyond limits

Jeremy Allison jra at samba.org
Mon Feb 18 23:57:17 UTC 2019


On Mon, Feb 18, 2019 at 06:22:49PM +0100, swen via samba-technical wrote:
> Hi Volker
> > 
> > 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

> 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));

This is *way* too complex to even understand as a comment.

Please just add the buffer overrun/overflow checks.

That way we *KNOW* it's safe and don't need to read
"War and Peace" to understand.

NAK until then !

Jeremy.



More information about the samba-technical mailing list