[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Marek Marczykowski-G??recki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
  • From: Ian Jackson <ian.jackson@xxxxxxxxxx>
  • Date: Wed, 19 Aug 2020 11:30:57 +0100
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Elliott Mitchell <ehem+xen@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Wei Liu" <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Delivery-date: Wed, 19 Aug 2020 10:31:25 +0000
  • Ironport-sdr: 4vKh1Zl9xUMMnfXu4oxN2NHUcaV2FFB9STVHE2rmPgbGa6rFi9VLL/QejBqz/Ys/V7+Vh6BvCb 8uIq+hisSNG0WbYY0qA+9wkCGBRj++bsnhQ+al6ifA1TxmiUtDnpML/TUuzuwbcBcemgSXFH/P VChVEFYM92sZO9GhtHdk3PCfcMlmijgjHR/bwJONRRs2/3MfRJTbphTzRz96Qwq+rDq+h6Tx5j wqw2mNCeyRg/QpSJLhSwsj9zhhnWzZzZT6+axuaWRge7espdv7YqAboWDaG+kDGXg8KSaEWBnZ 0Tk=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

Some systems insist on sun_path being nul-terminated, but I don't
think that includes any we care about.  AFAICT from the manpage
FreeBSD doesn't and uses a variable socklen for AF_UNIX.

OTOH I don't think there is much benefit in the additional byte so I
don't mind if we take some version of these changes.

I think Marek is right that his patch does leave sun_path
nul-terminated, so, for that original patch:

Reviewed-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Thanks,
Ian.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.