PATCH: ctdb: buffer write beyond limits

[Martin Schwenke]

On Wed, 20 Feb 2019 13:46:14 -0700, Christof Schmitt <cs at samba.org>
wrote:

> On Wed, Feb 20, 2019 at 02:11:18PM +1100, Martin Schwenke via samba-technical wrote:
> > 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:
> > >   
>  [...]  
>  [...]  
>  [...]  
> > > 
> > > 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:  
> 
> The irony... Thanks you for the fix, that looks correct.
> 
> > > 
> > > 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  
> 
> Yes, here are the fix with my reviewed-by and the updated test patch.
> Besides your fix i also had to fix the dependency of the new test on
> tdb; otherwise a compile on a system without tdb.h fails.
> I started a pipeline run at
> https://gitlab.com/samba-team/devel/samba/pipelines/48450894

All looks good.  Thanks again for the test!

Reviewed-by: Martin Schwenke <martin at meltin.net>

Pushed to autobuild...

peace & happiness,
martin