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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 7 Oct 2020, at 10:14, Jürgen Groß <jgross@xxxxxxxx> wrote:
> 
> 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?

it is potentially more computation (complexity was not the right word maybe).

> 
> In the end both variants are fine.

in the end I am fully ok with any, at then end there is no performance issue 
here.
I will use use the memset solution and push a v3.

Cheers
Bertrand

> 
> 
> Juergen