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