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.jack...@eu.citrix.com>
Thanks,
Ian.
