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

[Marek Marczykowski-G??recki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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.
>
> 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.

unix(7) indeed says it varies across implementations and for example
Linux would add the nul byte itself (being 109th character there). But
it generally recommends to include the nul byte to avoid hitting some
corner cases (an example given there is getsockname() returning larger
buffer than normally, to accommodate that one extra byte).

> 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.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature