Re: [PATCH 2/2] libxl: fix -Werror=stringop-truncation in libxl__prepare_sockaddr_un

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Wed, Aug 19, 2020 at 11:30:57AM +0100, Ian Jackson wrote:
> Marek Marczykowski-G??recki writes ("Re: [PATCH 2/2] libxl: fix 
> -Werror=stringop-truncation in libxl__prepare_sockaddr_un"):
> > On Tue, Aug 18, 2020 at 08:43:56PM -0700, Elliott Mitchell wrote:
> > > On Wed, Aug 19, 2020 at 04:00:36AM +0200, Marek Marczykowski-G??recki 
> > > wrote:
> > > > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > > > index f360f5e228..b039143b8a 100644
> > > > --- a/tools/libxl/libxl_utils.c
> > > > +++ b/tools/libxl/libxl_utils.c
> > > 
> > > 
> > > >      }
> > > >      memset(un, 0, sizeof(struct sockaddr_un));
> > > >      un->sun_family = AF_UNIX;
> > > > -    strncpy(un->sun_path, path, sizeof(un->sun_path));
> > > > +    strncpy(un->sun_path, path, sizeof(un->sun_path) - 1);
> > > >      return 0;
> > > >  }
> > > 
> > > While the earlier lines are okay, this line introduces an error.  
> > 
> > Why exactly? strncpy() copies up to n characters, quoting its manual
> > page:
> > 
> >     If there is no null byte among the first n bytes of src, the string
> >     placed in dest will not be null-terminated
> > 
> > But since the whole struct is zeroed out initially, this should still
> > result in a null terminated string, as the last byte of that buffer will
> > not be touched by the strncpy.
> 
> Everyone here so far, including the compiler, seems to be assuming
> that sun_path must be nul-terminated.  But that is not strictly
> correct.  So the old code is not buggy and the compiler is wrong.

For portability it /should/ be nul-terminated.  According to the man
pages for both Linux and FreeBSD, neither actually depends on the
nul-byte.

I would argue for using either strcpy(), memcpy(), or merging things
together with strlcpy().

Rereading, the log message is "Path must be less than...".  If it said
"no more than", then subtracting 1 would be correct, but with "less" it
should state the buffer length.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445