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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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

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

Cheers
Bertrand

> 
> 
> Juergen