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

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

[Bertrand Marquis]

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@arm.com>
---
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
+					 */
+					memcpy(result, de->d_name, MIN(strnlen(de->d_name,
+									sizeof(de->d_name)),resultLen - 1));
 					result[resultLen - 1] = 0;
 				}
 		}
@@ -264,7 +271,7 @@ int xenstat_collect_networks(xenstat_node * node)
 {
 	/* Helper variables for parseNetDevLine() function defined above */
 	int i;
-	char line[512] = { 0 }, iface[16] = { 0 }, devBridge[256] = { 0 }, devNoBridge[257] = { 0 };
+	char line[512] = { 0 }, iface[16] = { 0 }, devBridge[16] = { 0 }, devNoBridge[17] = { 0 };
 	unsigned long long rxBytes, rxPackets, rxErrs, rxDrops, txBytes, txPackets, txErrs, txDrops;
 
 	struct priv_data *priv = get_priv_data(node->handle);
-- 
2.17.1

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

[Jürgen Groß]

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@arm.com>
> ---
> 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/

> +					 */
> +					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.


Juergen

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

[Bertrand Marquis]

Hi Jurgen,

> On 7 Oct 2020, at 09:39, Jürgen Groß <jgross@suse.com> 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@arm.com>
>> ---
>> 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

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

[Jürgen Groß]

On 07.10.20 10:56, Bertrand Marquis wrote:
> Hi Jurgen,
> 
>> On 7 Oct 2020, at 09:39, Jürgen Groß <jgross@suse.com> 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@arm.com>
>>> ---
>>> 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

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

[Bertrand Marquis]

> On 7 Oct 2020, at 10:14, Jürgen Groß <jgross@suse.com> wrote:
> 
> On 07.10.20 10:56, Bertrand Marquis wrote:
>> Hi Jurgen,
>>> On 7 Oct 2020, at 09:39, Jürgen Groß <jgross@suse.com> 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@arm.com>
>>>> ---
>>>> 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