| Message ID | 20200819020036.599065-2-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] libxl: workaround gcc 10.2 maybe-uninitialized warning | expand |
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. If the compiler complains once the earlier lines are fixed, something might need to be done about this. I would be tempted to switch to strlcpy() since this doesn't look like it needs un->sun_path to be zeroed out. (note I'm not familiar with this code, so someone else needs to check me on this)
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.
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@eu.citrix.com>
Thanks,
Ian.
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@eu.citrix.com> > > Thanks, > Ian.
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.
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 @@ -1252,14 +1252,14 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc, struct sockaddr_un *un, const char *path, const char *what) { - if (sizeof(un->sun_path) <= strlen(path)) { + if (sizeof(un->sun_path) - 1 <= strlen(path)) { LOG(ERROR, "UNIX socket path '%s' is too long for %s", path, what); - LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path)); + LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path) - 1); return ERROR_INVAL; } 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; }
In file included from /usr/include/string.h:495, from libxl_internal.h:38, from libxl_utils.c:20: In function 'strncpy', inlined from 'libxl__prepare_sockaddr_un' at libxl_utils.c:1262:5: /usr/include/bits/string_fortified.h:106:10: error: '__builtin_strncpy' specified bound 108 equals destination size [-Werror=stringop-truncation] 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- tools/libxl/libxl_utils.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)