[PATCH 1/2] libxl: workaround gcc 10.2 maybe-uninitialized warning

[PATCH 1/2] libxl: workaround gcc 10.2 maybe-uninitialized warning

[Marek Marczykowski-Górecki]

It seems xlu_pci_parse_bdf has a state machine that is too complex for
gcc to understand. The build fails with:

    libxlu_pci.c: In function 'xlu_pci_parse_bdf':
    libxlu_pci.c:32:18: error: 'func' may be used uninitialized in this function [-Werror=maybe-uninitialized]
       32 |     pcidev->func = func;
          |     ~~~~~~~~~~~~~^~~~~~
    libxlu_pci.c:51:29: note: 'func' was declared here
       51 |     unsigned dom, bus, dev, func, vslot = 0;
          |                             ^~~~
    libxlu_pci.c:31:17: error: 'dev' may be used uninitialized in this function [-Werror=maybe-uninitialized]
       31 |     pcidev->dev = dev;
          |     ~~~~~~~~~~~~^~~~~
    libxlu_pci.c:51:24: note: 'dev' was declared here
       51 |     unsigned dom, bus, dev, func, vslot = 0;
          |                        ^~~
    libxlu_pci.c:30:17: error: 'bus' may be used uninitialized in this function [-Werror=maybe-uninitialized]
       30 |     pcidev->bus = bus;
          |     ~~~~~~~~~~~~^~~~~
    libxlu_pci.c:51:19: note: 'bus' was declared here
       51 |     unsigned dom, bus, dev, func, vslot = 0;
          |                   ^~~
    libxlu_pci.c:29:20: error: 'dom' may be used uninitialized in this function [-Werror=maybe-uninitialized]
       29 |     pcidev->domain = domain;
          |     ~~~~~~~~~~~~~~~^~~~~~~~
    libxlu_pci.c:51:14: note: 'dom' was declared here
       51 |     unsigned dom, bus, dev, func, vslot = 0;
          |              ^~~
    cc1: all warnings being treated as errors

Workaround it by setting the initial value to invalid value (0xffffffff)
and then assert on each value being set. This way we mute the gcc
warning, while still detecting bugs in the parse code.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxlu_pci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxlu_pci.c b/tools/libxl/libxlu_pci.c
index 7947687661..e2709c5f89 100644
--- a/tools/libxl/libxlu_pci.c
+++ b/tools/libxl/libxlu_pci.c
@@ -45,10 +45,11 @@ static int pcidev_struct_fill(libxl_device_pci *pcidev, unsigned int domain,
 #define STATE_TYPE      9
 #define STATE_RDM_STRATEGY      10
 #define STATE_RESERVE_POLICY    11
+#define INVALID         0xffffffff
 int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str)
 {
     unsigned state = STATE_DOMAIN;
-    unsigned dom, bus, dev, func, vslot = 0;
+    unsigned dom = INVALID, bus = INVALID, dev = INVALID, func = INVALID, vslot = 0;
     char *buf2, *tok, *ptr, *end, *optkey = NULL;
 
     if ( NULL == (buf2 = ptr = strdup(str)) )
@@ -170,6 +171,8 @@ int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str
     if ( tok != ptr || state != STATE_TERMINAL )
         goto parse_error;
 
+    assert(dom != INVALID && bus != INVALID && dev != INVALID && func != INVALID);
+
     /* Just a pretty way to fill in the values */
     pcidev_struct_fill(pcidev, dom, bus, dev, func, vslot << 3);
 
-- 
2.25.4

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

[Marek Marczykowski-Górecki]

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(-)

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;
 }
 
-- 
2.25.4

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

[Elliott Mitchell]

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)


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

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

[Marek Marczykowski-G??recki]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

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.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ian Jackson]

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.

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

[Marek Marczykowski-G??recki]

[-- Attachment #1: Type: text/plain, Size: 2617 bytes --]

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.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Elliott Mitchell]

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@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH 1/2] libxl: workaround gcc 10.2 maybe-uninitialized warning

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[PATCH 1/2] libxl: workaround gcc 10.2 maybe-uninitialized warning"):
> It seems xlu_pci_parse_bdf has a state machine that is too complex for
> gcc to understand. The build fails with:

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>