Fix multiple compiler warnings when building with gcc-12

[John Paul Adrian Glaubitz]

This patch series fixes a number of warnings when building powerpc-utils
with gcc-12 or newer. Since the project builds with "-Werror" by default,
these warnings will cause the build to fail.

With these patches applied, all warnings are gone when building on ppc64el,
there are two additional warnings left regarding possibly truncated strings
when building on big-endian PowerPC targets for which I will send a separate
patch set.

Cheers,
Adrian

--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

[John Paul Adrian Glaubitz]

This fixes the following compiler warning when building with gcc-12:

In file included from /usr/include/stdio.h:906,
from src/vcpustat.c:26:
In function 'printf',
inlined from 'print_stats' at src/vcpustat.c:182:4:
/usr/include/powerpc64-linux-gnu/bits/stdio2.h:86:10: error: 'stat.far_numa_node' may be used uninitialized [-Werror=maybe-uninitialized]
86 | return __printf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, __va_arg_pack ());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/vcpustat.c: In function 'print_stats':
src/vcpustat.c:144:34: note: 'stat.far_numa_node' was declared here
144 | struct vcpudispatch_stat stat;
| ^~~~

Signed-off-by: John Paul Adrian Glaubitz <glau...@physik.fu-berlin.de>
---
src/vcpustat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vcpustat.c b/src/vcpustat.c
index 415846f..d17afe7 100644
--- a/src/vcpustat.c
+++ b/src/vcpustat.c
@@ -141,7 +141,7 @@ void print_stats(struct vcpudispatch_stat stats1[],
char raw_header1[] = "%22s %43s | %32s\n";
char raw_header2[] = "%-7s | %10s | %10s %10s %10s %10s | %10s %10s %10s\n";
char raw_fmt[] = "cpu%-4d | %10d | %10d %10d %10d %10d | %10d %10d %10d\n";
- struct vcpudispatch_stat stat;
+ struct vcpudispatch_stat stat = {};
int i;

if (stats_off) {
--
2.30.2

[John Paul Adrian Glaubitz]

Ping. Any chance we can get these small fixes merged for the next branch?

Thanks,
Adrian

[Nathan Lynch]

John Paul Adrian Glaubitz <glau...@physik.fu-berlin.de> writes:

> Ping. Any chance we can get these small fixes merged for the next
> branch?

Tyrel, I think these all look good. Thanks Adrian.

We should really remove the -Werror from the default build flags, and
instead use it judiciously in the CI.

[Tyrel Datwyler]

Agreed. Thanks Nathan for the immediate turn around on patches to do just that.

-Tyrel

[Tyrel Datwyler]

On 12/26/22 01:54, John Paul Adrian Glaubitz wrote:
> This patch series fixes a number of warnings when building powerpc-utils
> with gcc-12 or newer. Since the project builds with "-Werror" by default,
> these warnings will cause the build to fail.
>
> With these patches applied, all warnings are gone when building on ppc64el,
> there are two additional warnings left regarding possibly truncated strings
> when building on big-endian PowerPC targets for which I will send a separate
> patch set.
>
> Cheers,
> Adrian
>
> --

Patchset applied to powerpc-utils/next branch:

[1/3]
https://github.com/ibm-power-utilities/powerpc-utils/commit/aef8f14ed8b241ab66d88fb9a5aeefe47a847267

[2/3]
https://github.com/ibm-power-utilities/powerpc-utils/commit/5de0a4a070981b5ee005f2242b31db5422be297a

[3/3]
https://github.com/ibm-power-utilities/powerpc-utils/commit/3607e6dabdef641c363233eddd3a1cf8c2e5c6d8

Thanks,
-Tyrel

[John Paul Adrian Glaubitz]

Hi Tyrel!

On 1/5/23 01:33, Tyrel Datwyler wrote:
> Patchset applied to powerpc-utils/next branch:
>
> [1/3]
> https://github.com/ibm-power-utilities/powerpc-utils/commit/aef8f14ed8b241ab66d88fb9a5aeefe47a847267
>
> [2/3]
> https://github.com/ibm-power-utilities/powerpc-utils/commit/5de0a4a070981b5ee005f2242b31db5422be297a
>
> [3/3]
> https://github.com/ibm-power-utilities/powerpc-utils/commit/3607e6dabdef641c363233eddd3a1cf8c2e5c6d8

Thanks a lot, much appreciated!

[Tyrel Datwyler]

On 12/26/22 01:54, John Paul Adrian Glaubitz wrote:

> This patch series fixes a number of warnings when building powerpc-utils
> with gcc-12 or newer. Since the project builds with "-Werror" by default,
> these warnings will cause the build to fail.
>
> With these patches applied, all warnings are gone when building on ppc64el,
> there are two additional warnings left regarding possibly truncated strings
> when building on big-endian PowerPC targets for which I will send a separate
> patch set.

I ran our CI with the latest Ubuntu LTS runner which has the gcc-11 toolchain
and I see the two truncated string warnings with that toolchain.

-Tyrel

[John Paul Adrian Glaubitz]

Hi Tyrel!

On 1/5/23 21:58, Tyrel Datwyler wrote:
> On 12/26/22 01:54, John Paul Adrian Glaubitz wrote:
>> This patch series fixes a number of warnings when building powerpc-utils
>> with gcc-12 or newer. Since the project builds with "-Werror" by default,
>> these warnings will cause the build to fail.
>>
>> With these patches applied, all warnings are gone when building on ppc64el,
>> there are two additional warnings left regarding possibly truncated strings
>> when building on big-endian PowerPC targets for which I will send a separate
>> patch set.
>
> I ran our CI with the latest Ubuntu LTS runner which has the gcc-11 toolchain
> and I see the two truncated string warnings with that toolchain.

So, these two warnings actually go away when I replace strncpy() with memcpy()
but I have to admit, I don't fully understand why that's the case.

diff --git a/src/errinjct/ioa_bus_error.c b/src/errinjct/ioa_bus_error.c
index 9d85cfa..5ee1401 100644
--- a/src/errinjct/ioa_bus_error.c
+++ b/src/errinjct/ioa_bus_error.c
@@ -204,7 +204,7 @@ static uint32_t get_config_addr_from_reg(char *devpath)
uint32_t *be_caddr;
uint32_t caddr = 0;

- strncpy(path, devpath, BUFSZ-5);
+ memcpy(path, devpath, BUFSZ-5);
strcat(path, "/reg");

buf = read_file(path, NULL);
diff --git a/src/serv_config.c b/src/serv_config.c
index 00ab672..2565533 100644
--- a/src/serv_config.c
+++ b/src/serv_config.c
@@ -707,7 +707,7 @@ retrieve_value(struct service_var *var, char *buf, size_t size) {
byte_to_string(param[2], buf, size);
}
else {
- strncpy(buf, param+2, ((size>ret_size)?
+ memcpy(buf, param+2, ((size>ret_size)?
ret_size:size));
buf[ret_size] = '\0';
}

If you're fine with that change, I can post a patch to the mailing list.

[Tyrel Datwyler]

On 1/5/23 13:07, John Paul Adrian Glaubitz wrote:
> Hi Tyrel!
>
> On 1/5/23 21:58, Tyrel Datwyler wrote:
>> On 12/26/22 01:54, John Paul Adrian Glaubitz wrote:
>>> This patch series fixes a number of warnings when building powerpc-utils
>>> with gcc-12 or newer. Since the project builds with "-Werror" by default,
>>> these warnings will cause the build to fail.
>>>
>>> With these patches applied, all warnings are gone when building on ppc64el,
>>> there are two additional warnings left regarding possibly truncated strings
>>> when building on big-endian PowerPC targets for which I will send a separate
>>> patch set.
>>
>> I ran our CI with the latest Ubuntu LTS runner which has the gcc-11 toolchain
>> and I see the two truncated string warnings with that toolchain.
>
> So, these two warnings actually go away when I replace strncpy() with memcpy()
> but I have to admit, I don't fully understand why that's the case.

In cases where the compiler can extrapolate the size of the source buffer and
the value or value range of `n` we get a possible truncation warning when `n` or
the range of `n` is less than the length of the source buffer.

By using memcpy the compiler no longer assumes the source is a string and we
are claiming we know what we are doing and that the exact range of `n` should be
copied. This should probably raise some eyebrows with regards to string
handling. To use memcpy we need be explicitly terminating or ensuring by some
means that a string range copied with memcpy is guaranteed to be NULL terminated.

A lot of this code base I think has implicit assumptions about what a reasonably
big buffer size should be to prevent overrun-truncation.

So, I'm a little hesitant to switch to memcpy to silence these warnings as
digging deeper into them has revealed some unexpectedly subtle bugs.

>
> diff --git a/src/errinjct/ioa_bus_error.c b/src/errinjct/ioa_bus_error.c
> index 9d85cfa..5ee1401 100644
> --- a/src/errinjct/ioa_bus_error.c
> +++ b/src/errinjct/ioa_bus_error.c
> @@ -204,7 +204,7 @@ static uint32_t get_config_addr_from_reg(char *devpath)
>         uint32_t *be_caddr;
>         uint32_t caddr = 0;
>  
> -       strncpy(path, devpath, BUFSZ-5);
> +       memcpy(path, devpath, BUFSZ-5);
>         strcat(path, "/reg");

The caller passes the devpath buffer which is allocated statically as BUFSZ.
Since, we try to copy only BUFSZ-5 to ensure we have space to strcat the string
"/reg\0" we get the truncation warning. Using memcpy here or changing the static
allocation size of devpath in the caller to BUFSZ-5 both make the warning go away.

Second to this however is that the errinjct code defines BUFSZ as 4000. This is
undersized if we somehow ever hit a maximum filepath on Linux where PATH_MAX is
4096.

>  
>         buf = read_file(path, NULL);
> diff --git a/src/serv_config.c b/src/serv_config.c
> index 00ab672..2565533 100644
> --- a/src/serv_config.c
> +++ b/src/serv_config.c
> @@ -707,7 +707,7 @@ retrieve_value(struct service_var *var, char *buf, size_t
> size) {
>                                 byte_to_string(param[2], buf, size);
>                         }
>                         else {
> -                               strncpy(buf, param+2, ((size>ret_size)?
> +                               memcpy(buf, param+2, ((size>ret_size)?
>                                         ret_size:size));
>                                 buf[ret_size] = '\0';
>                         }

This one is interesting and is possibly a good candidate for being switched over
to memcpy. The compiler knows `param` is statically allocated as BUF_SIZE, and
that the value of `size` passed in by the caller is BUF_SIZE. The first two
bytes of param are the actual string length of the string the makes up the rest
of the param buffer and is copied into `ret_size` variable. So, `param+2` is a
buffer of size BUF_SIZE-2 which is less than BUF_SIZE and when we use `size`
with strncpy it actually copies 2 random bytes passed the end of `param` into
`buf` if no NULL terminator is found. I haven't dug deep enough to determine if
the string returned in `param` is guaranteed to be NULL terminated, but
considering there is an explicit NULL termination in the code I don't think it
is safe to assume.

The most interesting part is why the compiler thinks `ret_size` is a range of
0-255 when it is a uint16, and creates the truncation warning since the range is
smaller than BUF_SIZE-2.

690 ret_size = be16toh(*param);

This byte swap code found a little before is wrong and will only store the first
byte of `param` since param is defined as a char array. So, this code is
actually broken if a parameter string longer than 255 characters is ever returned.

Finally, the ternary operator makes more sense in the NULL termination
expression rather than the strncpy expression. For strncpy or memcpy it suffices
to copy the source buffer size (BUF_SIZE-2 in this case) if we are doing
explicit NULL termination following.

-Tyrel

[John Paul Adrian Glaubitz]

Hi Tyrel!

Thanks a lot for the elaborate explanation. That helps understanding the problem!

The byte-swap issue would explain why this warning occurs on big-endian PowerPC targets
only. I guess, if we fix this one, we should be able to make the compiler happy about
the string operations.