[Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[christopher.w.clark]

From: Christopher Clark <christopher.w.clark@gmail.com>

gcc 9.1.0 reports:

| test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
|    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
|       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| test-cpu-policy.c:20:12: note: in definition of macro 'fail'
|    20 |     printf(fmt, ##__VA_ARGS__);                 \
|       |            ^~~
| test-cpu-policy.c:64:27: note: format string is defined here
|    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
|       |                           ^~~~~
| test-cpu-policy.c:44:7: note: referenced argument declared here
|    44 |     } tests[] = {
|       |       ^~~~~

so increase the string array size by one character for the null string
terminator and add another single char to the struct within the same
union to balance it.

Signed-off-by: Christopher Clark <christopher.clark6@baesystems.com>
---
 tools/tests/cpu-policy/test-cpu-policy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index ca3b8dd45f..c91408a93a 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -35,9 +35,10 @@ static void test_vendor_identification(void)
 {
     static const struct test {
         union {
-            char ident[12];
+            char ident[13];
             struct {
                 uint32_t b, d, c;
+                char null_terminator;
             };
         };
         unsigned int vendor;
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Roger Pau Monné]

On Wed, Jul 24, 2019 at 05:53:26PM -0700, christopher.w.clark@gmail.com wrote:
> From: Christopher Clark <christopher.w.clark@gmail.com>
> 
> gcc 9.1.0 reports:
> 
> | test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
> |       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | test-cpu-policy.c:20:12: note: in definition of macro 'fail'
> |    20 |     printf(fmt, ##__VA_ARGS__);                 \
> |       |            ^~~
> | test-cpu-policy.c:64:27: note: format string is defined here
> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
> |       |                           ^~~~~
> | test-cpu-policy.c:44:7: note: referenced argument declared here
> |    44 |     } tests[] = {
> |       |       ^~~~~
> 
> so increase the string array size by one character for the null string
> terminator and add another single char to the struct within the same
> union to balance it.
> 
> Signed-off-by: Christopher Clark <christopher.clark6@baesystems.com>

Thanks! Just two nits below.

> ---
>  tools/tests/cpu-policy/test-cpu-policy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
> index ca3b8dd45f..c91408a93a 100644
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -35,9 +35,10 @@ static void test_vendor_identification(void)
>  {
>      static const struct test {
>          union {
> -            char ident[12];
> +            char ident[13];
>              struct {
>                  uint32_t b, d, c;
> +                char null_terminator;

Do you really need this field here, AFAICT it's unused. The compiler
will add the padding here automatically to match the size of the other
field of the union.

Also, since ident is null terminated now you can remove the .12 from
the format string.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Jan Beulich]

On 25.07.2019 02:53, christopher.w.clark@gmail.com wrote:
> gcc 9.1.0 reports:
> 
> | test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
> |       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | test-cpu-policy.c:20:12: note: in definition of macro 'fail'
> |    20 |     printf(fmt, ##__VA_ARGS__);                 \
> |       |            ^~~
> | test-cpu-policy.c:64:27: note: format string is defined here
> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
> |       |                           ^~~~~
> | test-cpu-policy.c:44:7: note: referenced argument declared here
> |    44 |     } tests[] = {
> |       |       ^~~~~

I must be missing something here: %.12s says that no more than 12
bytes are to be read from the string. There's nul terminator
required. This is what the standard says

"Characters from the array are written up to (but not including)
  the terminating null character. If the precision is specified, no
  more than that many bytes are written. If the precision is not
  specified or is greater than the size of the array, the array
  shall contain a null character."

For the moment it looks to me as if the compiler was wrong to
complain.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Andrew Cooper]

On 25/07/2019 11:03, Jan Beulich wrote:
> On 25.07.2019 02:53, christopher.w.clark@gmail.com wrote:
>> gcc 9.1.0 reports:
>>
>> | test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
>> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
>> |       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | test-cpu-policy.c:20:12: note: in definition of macro 'fail'
>> |    20 |     printf(fmt, ##__VA_ARGS__);                 \
>> |       |            ^~~
>> | test-cpu-policy.c:64:27: note: format string is defined here
>> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
>> |       |                           ^~~~~
>> | test-cpu-policy.c:44:7: note: referenced argument declared here
>> |    44 |     } tests[] = {
>> |       |       ^~~~~
> I must be missing something here: %.12s says that no more than 12
> bytes are to be read from the string. There's nul terminator
> required. This is what the standard says
>
> "Characters from the array are written up to (but not including)
>   the terminating null character. If the precision is specified, no
>   more than that many bytes are written. If the precision is not
>   specified or is greater than the size of the array, the array
>   shall contain a null character."
>
> For the moment it looks to me as if the compiler was wrong to
> complain.

That is my assessment as well, but I haven't had time to file a GCC bug yet.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1406 bytes --]

On Thu, 2019-07-25 at 10:25 +0200, Roger Pau Monné wrote:
> On Wed, Jul 24, 2019 at 05:53:26PM -0700, 
> christopher.w.clark@gmail.com wrote:
> > From: Christopher Clark <christopher.w.clark@gmail.com>
> > 
> > diff --git a/tools/tests/cpu-policy/test-cpu-policy.c
> > b/tools/tests/cpu-policy/test-cpu-policy.c
> > index ca3b8dd45f..c91408a93a 100644
> > --- a/tools/tests/cpu-policy/test-cpu-policy.c
> > +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> > @@ -35,9 +35,10 @@ static void test_vendor_identification(void)
> >  {
> >      static const struct test {
> >          union {
> > -            char ident[12];
> > +            char ident[13];
> >              struct {
> >                  uint32_t b, d, c;
> > +                char null_terminator;
> 
> Do you really need this field here, AFAICT it's unused. The compiler
> will add the padding here automatically to match the size of the
> other
> field of the union.
> 
Yep, I run into this myself (and sent a mail about it yesterday), and,
FWIW, I confirm that just growing the array to 13 both builds, with
GCC9, and works. :-)

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Jan Beulich]

On 25.07.2019 02:53, christopher.w.clark@gmail.com wrote:
> From: Christopher Clark <christopher.w.clark@gmail.com>
> 
> gcc 9.1.0 reports:
> 
> | test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
> |       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | test-cpu-policy.c:20:12: note: in definition of macro 'fail'
> |    20 |     printf(fmt, ##__VA_ARGS__);                 \
> |       |            ^~~
> | test-cpu-policy.c:64:27: note: format string is defined here
> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
> |       |                           ^~~~~
> | test-cpu-policy.c:44:7: note: referenced argument declared here
> |    44 |     } tests[] = {
> |       |       ^~~~~

In order to possibly create a bug report against gcc I've tried this:

#include <stdio.h>

struct s {
	char ac[12];
	int i;
};

void test(const struct s*ps) {
	printf("'%.12s'\n", ps->ac);
}

There's no warning here. Could you check whether the compiler warns on
that simple test for you? If it does - are we talking about plain
upstream 9.1.0 (in which case I'd be really puzzled by the difference
in behavior)?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Andrew Cooper]

On 30/07/2019 16:51, Jan Beulich wrote:
> On 25.07.2019 02:53, christopher.w.clark@gmail.com wrote:
>> From: Christopher Clark <christopher.w.clark@gmail.com>
>>
>> gcc 9.1.0 reports:
>>
>> | test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
>> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
>> |       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> | test-cpu-policy.c:20:12: note: in definition of macro 'fail'
>> |    20 |     printf(fmt, ##__VA_ARGS__);                 \
>> |       |            ^~~
>> | test-cpu-policy.c:64:27: note: format string is defined here
>> |    64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
>> |       |                           ^~~~~
>> | test-cpu-policy.c:44:7: note: referenced argument declared here
>> |    44 |     } tests[] = {
>> |       |       ^~~~~
> In order to possibly create a bug report against gcc I've tried this:
>
> #include <stdio.h>
>
> struct s {
> 	char ac[12];
> 	int i;
> };
>
> void test(const struct s*ps) {
> 	printf("'%.12s'\n", ps->ac);
> }
>
> There's no warning here. Could you check whether the compiler warns on
> that simple test for you? If it does - are we talking about plain
> upstream 9.1.0 (in which case I'd be really puzzled by the difference
> in behavior)?

CC Dario who also reported this build failure.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4986 bytes --]

On Tue, 2019-07-30 at 17:23 +0100, Andrew Cooper wrote:
> On 30/07/2019 16:51, Jan Beulich wrote:
> > On 25.07.2019 02:53, christopher.w.clark@gmail.com wrote:
> > > From: Christopher Clark <christopher.w.clark@gmail.com>
> > > 
> > > gcc 9.1.0 reports:
> > > 
> > > > test-cpu-policy.c:64:18: error: '%.12s' directive argument is
> > > > not a nul-terminated string [-Werror=format-overflow=]
> > > >    64 |             fail("  Test '%.12s', expected vendor %u,
> > > > got %u\n",
> > > >       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ~~~~~~~~
> > > > test-cpu-policy.c:20:12: note: in definition of macro 'fail'
> > > >    20 |     printf(fmt, ##__VA_ARGS__);                 \
> > > >       |            ^~~
> > > > test-cpu-policy.c:64:27: note: format string is defined here
> > > >    64 |             fail("  Test '%.12s', expected vendor %u,
> > > > got %u\n",
> > > >       |                           ^~~~~
> > > > test-cpu-policy.c:44:7: note: referenced argument declared here
> > > >    44 |     } tests[] = {
> > > >       |       ^~~~~
> > In order to possibly create a bug report against gcc I've tried
> > this:
> > 
> > #include <stdio.h>
> > 
> > struct s {
> > 	char ac[12];
> > 	int i;
> > };
> > 
> > void test(const struct s*ps) {
> > 	printf("'%.12s'\n", ps->ac);
> > }
> > 
> > There's no warning here. Could you check whether the compiler warns
> > on
> > that simple test for you? If it does - are we talking about plain
> > upstream 9.1.0 (in which case I'd be really puzzled by the
> > difference
> > in behavior)?
> 
> CC Dario who also reported this build failure.
> 
Yep, this thread was on my radar already. But thanks! :-P

So, I'm using the following gcc (stock opensuse Tumbleweed provided
one):

dario@Palanthas:~/src/xen/xen.git> gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/9/lto-wrapper
OFFLOAD_TARGET_NAMES=hsa:nvptx-none
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,objc,fortran,obj-c++,ada,go,d --enable-offload-targets=hsa,nvptx-none=/usr/nvptx-none, --without-cuda-driver --disable-werror --with-gxx-include-dir=/usr/include/c++/9 --enable-ssp --disable-libssp --disable-libvtv --disable-cet --disable-libcc1 --enable-plugin --with-bugurl=https://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --with-slibdir=/lib64 --with-system-zlib --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --enable-libphobos --enable-version-specific-runtime-libs --with-gcc-major-version-only --enable-linker-build-id --enable-linux-futex --enable-gnu-indirect-function --program-suffix=-9 --without-system-libunwind --enable-multilib --with-arch-32=x86-64 --with-tune=generic --with-build-config=bootstrap-lto-lean --enable-link-mutex --build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
gcc version 9.1.1 20190703 [gcc-9-branch revision 273008] (SUSE Linux)


Jan's example above, seem to compile **without any warnings** for me as
well. If I add a main(), I can even get the code above to print the
content of the array.

And yet, building the tools without a patch like Christoper's one
(which was also what I was using locally, and raised to Andy), I get:

make[6]: Entering directory '/build/tools/tests/cpu-policy'
gcc  -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -g3 -O0 -fno-omit-frame-pointer -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF .test-cpu-policy.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE   -Werror -I/build/tools/tests/cpu-policy/../../../tools/include -D__XEN_TOOLS__ -O3   -c -o test-cpu-policy.o test-cpu-policy.c 
test-cpu-policy.c: In function 'main':
test-cpu-policy.c:64:18: error: '%.12s' directive argument is not a nul-terminated string [-Werror=format-overflow=]
   64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test-cpu-policy.c:20:12: note: in definition of macro 'fail'
   20 |     printf(fmt, ##__VA_ARGS__);                 \
      |            ^~~
test-cpu-policy.c:64:27: note: format string is defined here
   64 |             fail("  Test '%.12s', expected vendor %u, got %u\n",
      |                           ^~~~~
test-cpu-policy.c:44:7: note: referenced argument declared here
   44 |     } tests[] = {
      |       ^~~~~

I'm happy to do more tests.

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Jan Beulich]

On 31.07.2019 02:22, Dario Faggioli wrote:
> Jan's example above, seem to compile **without any warnings** for me as
> well. If I add a main(), I can even get the code above to print the
> content of the array.
> 
> And yet, building the tools without a patch like Christoper's one
> (which was also what I was using locally, and raised to Andy), I get:

Sure - I'm aware that we're using a (potentially heavily) patched gcc.
Hence my question to Christopher whether he's using a plain one.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tests/cpu-policy: fix format-overflow warning by null terminating strings

[Rich Persaud]

[-- Attachment #1.1: Type: text/plain, Size: 1136 bytes --]

> On Jul 31, 2019, at 04:11, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> On 31.07.2019 02:22, Dario Faggioli wrote:
>> Jan's example above, seem to compile **without any warnings** for me as
>> well. If I add a main(), I can even get the code above to print the
>> content of the array.
>> 
>> And yet, building the tools without a patch like Christoper's one
>> (which was also what I was using locally, and raised to Andy), I get:
> 
> Sure - I'm aware that we're using a (potentially heavily) patched gcc.
> Hence my question to Christopher whether he's using a plain one.

The Xen test-cpu-policy.c build error and workaround reported by Christopher can be reproduced by building Xen staging against upstream OpenEmbedded/Yocto meta-virtualization master and GCC.  I've tested on Devuan Ascii with "oe-build-xen x86-64 staging master":

  http://github.com/dozylynx/oe-build-xen

If there is an upstream GCC compiler issue, Xen will not be the only affected project.  We can expect new data as other projects start building with recent GCC versions.  In the meantime, the workaround can unblock Xen builds.

Rich

[-- Attachment #1.2: Type: text/html, Size: 2162 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel