PATCH: ctdb: buffer write beyond limits

[Martin Schwenke]

On Wed, 20 Feb 2019 13:59:15 +1100, Martin Schwenke via samba-technical
<samba-technical at lists.samba.org> wrote:

> On Tue, 19 Feb 2019 16:17:30 -0700, Christof Schmitt via
> samba-technical <samba-technical at lists.samba.org> wrote:
> 
> > On Tue, Feb 19, 2019 at 02:02:31PM -0700, Christof Schmitt via samba-technical wrote:  
> > > Maybe we can get this in another day? See the attached patch for the
> > > start of a test of the queue code. Could we extend that to trigger the
> > > problem and then verify that it is fixed with your patch?  
> > 
> > The initial patch was missing the wscript update. Here is an extended
> > version that shows the memory corruption without your fix. I would
> > suggest to add the fix with a test now to fix the 4.10 release and then
> > we can work towards adding more tests for the queueing code.  
> 
> Thanks for that.  That looks really good.  I always thought we would
> have problems writing unit tests for that code because it uses struct
> ctdb_context, which drags in the world, but this is nice!
> 
> Ironically, the test refuses to compile with CFLAGS="-O3" and
> --picky-developer:
> 
> [318/380] Compiling ctdb/tests/src/ctdb_io_test.c
> In file included from ../../tests/src/ctdb_io_test.c:23:
> ../../tests/src/ctdb_io_test.c: In function ‘test1_callback’:
> ../../tests/src/ctdb_io_test.c:70:9: error: ‘__builtin_memcmp_eq’ reading 12 bytes from a region of size 9 [-Werror=stringop-overflow=]
>   assert(memcmp(data  + sizeof(len), test1_req, len) == 0);
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> That is, the test reads off the end of the buffer...  :-)
> 
> The fixup below seems to do the job:
> 
> diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
> index 22bd2f4930f..5a2f81538a1 100644
> --- a/ctdb/tests/src/ctdb_io_test.c
> +++ b/ctdb/tests/src/ctdb_io_test.c
> @@ -67,7 +67,7 @@ static void test1_callback(uint8_t *data, size_t length, void *private_data)
>         assert(len == sizeof(uint32_t) + test1_req_len);
>  
>         assert(length == sizeof(uint32_t) + test1_req_len);
> -       assert(memcmp(data  + sizeof(len), test1_req, length) == 0);
> +       assert(memcmp(data  + sizeof(len), test1_req, test1_req_len) == 0);
>  }
>  
>  static void test1(void)

Oh, and if we're going to push it with the bug fix then can we please
add the bug tag?

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13791

Thanks...

peace & happiness,
martin