Re: [PATCH v3] tools/libs/stat: fix broken build

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 2 Oct 2020, at 11:44, Jürgen Groß <jgross@xxxxxxxx> wrote:
> 
> On 02.10.20 12:12, Bertrand Marquis wrote:
>> Hi,
>>> On 2 Oct 2020, at 08:25, Jürgen Groß <jgross@xxxxxxxx> wrote:
>>> 
>>> On 02.10.20 08:59, Jan Beulich wrote:
>>>> On 02.10.2020 08:51, Jürgen Groß wrote:
>>>>> On 02.10.20 08:20, Jan Beulich wrote:
>>>>>> On 02.10.2020 06:50, Jürgen Groß wrote:
>>>>>>> On 01.10.20 18:38, Bertrand Marquis wrote:
>>>>>>>> Hi Juergen,
>>>>>>>> 
>>>>>>>>> On 14 Sep 2020, at 11:58, Bertrand Marquis <bertrand.marquis@xxxxxxx> 
>>>>>>>>> wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> On 12 Sep 2020, at 14:08, Juergen Gross <jgross@xxxxxxxx> wrote:
>>>>>>>>>> 
>>>>>>>>>> Making getBridge() static triggered a build error with some gcc 
>>>>>>>>>> versions:
>>>>>>>>>> 
>>>>>>>>>> error: 'strncpy' output may be truncated copying 15 bytes from a 
>>>>>>>>>> string of
>>>>>>>>>> length 255 [-Werror=stringop-truncation]
>>>>>>>>>> 
>>>>>>>>>> Fix that by using a buffer with 256 bytes instead.
>>>>>>>>>> 
>>>>>>>>>> Fixes: 6d0ec053907794 ("tools: split libxenstat into new 
>>>>>>>>>> tools/libs/stat directory")
>>>>>>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>>>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>>>>>> 
>>>>>>>> Sorry i have to come back on this one.
>>>>>>>> 
>>>>>>>> I still see an error compiling with Yocto on this one:
>>>>>>>> |     inlined from 'xenstat_collect_networks' at xenstat_linux.c:306:2:
>>>>>>>> | xenstat_linux.c:81:6: error: 'strncpy' output may be truncated 
>>>>>>>> copying 255 bytes from a string of length 255 
>>>>>>>> [-Werror=stringop-truncation]
>>>>>>>> |    81 |      strncpy(result, de->d_name, resultLen);
>>>>>>>> |       |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>> 
>>>>>>>> To solve it, I need to define devBridge[257] as devNoBrideg.
>>>>>>> 
>>>>>>> IMHO this is a real compiler error.
>>>>>>> 
>>>>>>> de->d_name is an array of 256 bytes, so doing strncpy() from that to
>>>>>>> another array of 256 bytes with a length of 256 won't truncate anything.
>>>>>> 
>>>>>> That's a matter of how you look at it, I think: If the original array
>>>>>> doesn't hold a nul-terminated string, the destination array won't
>>>>>> either, yet the common goal of strncpy() is to yield a properly nul-
>>>>>> terminated string. IOW the warning may be since the standard even has
>>>>>> a specific foot note to point out this possible pitfall.
>>>>> 
>>>>> If the source doesn't hold a nul-terminated string there will still be
>>>>> 256 bytes copied, so there is no truncation done during strncpy().
>>>>> 
>>>>> In fact there is no way to use strncpy() in a safe way on a fixed sized
>>>>> source array with the above semantics: either the target is larger than
>>>>> the source and length is at least sizeof(source) + 1, resulting in a
>>>>> possible read beyond the end of source, or the target is the same length
>>>>> leading to the error.
>>>> I agree with all of what you say, but I can also see why said foot note
>>>> alone may have motivated the emission of the warning.
>>> 
>>> The motivation can be explained, yes, but it is wrong. strncpy() is not
>>> limited to source arrays of unknown length. So this warning is making
>>> strncpy() unusable for fixed sized source strings and -Werror. And that
>>> is nothing a compiler should be allowed to do, hence a compiler bug.
>> I do agree that in this case the compiler is doing to much.
> 
> It is plain wrong here. Rendering a Posix defined function unusable for
> a completely legal use case is in no way a matter of taste or of "doing
> to much". It is a bug.

Agree.

> 
>> We could also choose to turn off the warning either using pragma (which
>> i really do not like) or by adding a cflag for this specific file (but this 
>> might
>> hit us later in other places).
>> All in all this currently makes Xen master and staging not possible to
>> compile with Yocto so we need to find a solution as this will also
>> come in any distribution using a new compiler,
> 
> A variant you didn't mention would be open coding of strncpy() (or
> having a related inline function in a common header). This route would
> be the one I'd prefer in case the compiler guys insist on the behavior
> being fine.

True this possible, even though I do not like to modify the code that deeply
for one specific compiler.

> 
> You didn't tell us which compiler is being used and whether it really is
> up to date. A workaround might be to set EXTRA_CFLAGS_XEN_TOOLS to
> "-Wno-stringop-truncation" for the build.

That’s what i meant by “adding a cflag”, as you suggest we could also
make it global (and not limit it to this file).

The compiler I am using is the one from Yocto Gatesgarth (release coming in
october): gcc version 10.2.0 (released in july 2020).

Cheers

Bertrand