Re: [PATCH v2 1/2] tools: use memcpy instead of strncpy in getBridge

[Jürgen Groß <jgross@xxxxxxxx>]

On 07.10.20 10:56, Bertrand Marquis wrote:
> Hi Jurgen,
>
> > On 7 Oct 2020, at 09:39, Jürgen Groß <jgross@xxxxxxxx> wrote:
> >
> > On 07.10.20 10:28, Bertrand Marquis wrote:
> > > Use memcpy in getBridge to prevent gcc warnings about truncated
> > > strings. We know that we might truncate it, so the gcc warning
> > > here is wrong.
> > > Revert previous change changing buffer sizes as bigger buffers
> > > are not needed.
> > > Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > > ---
> > > Changes in v2:
> > >   Use MIN between string length of de->d_name and resultLen to copy only
> > >   the minimum size required and prevent crossing to from an unallocated
> > >   space.
> > > ---
> > >   tools/libs/stat/xenstat_linux.c | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > diff --git a/tools/libs/stat/xenstat_linux.c b/tools/libs/stat/xenstat_linux.c
> > > index d2ee6fda64..0ace03af1b 100644
> > > --- a/tools/libs/stat/xenstat_linux.c
> > > +++ b/tools/libs/stat/xenstat_linux.c
> > > @@ -29,6 +29,7 @@
> > >   #include <string.h>
> > >   #include <unistd.h>
> > >   #include <regex.h>
> > > +#include <xen-tools/libs.h>
> > >     #include "xenstat_priv.h"
> > >   @@ -78,7 +79,13 @@ static void getBridge(char *excludeName, char *result, 
> > > size_t resultLen)
> > >                                 sprintf(tmp, "/sys/class/net/%s/bridge", 
> > > de->d_name);
> > >                                 if (access(tmp, F_OK) == 0) {
> > > -                                       strncpy(result, de->d_name, resultLen);
> > > +                                       /*
> > > +                                        * Do not use strncpy to prevent 
> > > compiler warning with
> > > +                                        * gcc >= 10.0
> > > +                                        * If de->d_name is longer then 
> > > resultLen we truncate it
> > s/then/than/
> Will fix
>
> > > +                                        */
> > > +                                       memcpy(result, de->d_name, 
> > > MIN(strnlen(de->d_name,
> > > +                                                                       
> > > sizeof(de->d_name)),resultLen - 1));
> > You can't use sizeof(de->d_name) here, as AFAIK there is no guarantee
> > that de->d_name isn't e.g. defined like "char d_name[]".
> >
> > My suggestion to use NAME_MAX as upper boundary for the length was
> > really meant to be used that way.
> >
> > And additionally you might want to add 1 to the strnlen() result in
> > order to copy the trailing 0-byte, too (or you should zero out the
> > result buffer before and omit writing the final zero byte).
> >
> > Thinking more about it zeroing the result buffer is better as it even
> > covers the theoretical case of NAME_MAX being shorter than resultLen.
> Setting the result buffer completely to 0 and doing after a copy sounds like
> a big complexity.
>
> How about:
> copysize = MIN(strnlen(de->d_name,NAME_MAX), resultLen - 1);
> memcpy(result, de->d_name, copysize);
> result[copysize + 1] = 0
result[copysize] = 0;

> This would cover the case where NAME_MAX is shorter then resultLen.
Why is:

memset(result, 0, resultLen);
memcpy(result, de->d_name, MIN(strnlen(de->d_name,NAME_MAX), resultLen - 
1));
A big complexity?

In the end both variants are fine.


Juergen