Discussion:
Fix multiple compiler warnings when building with gcc-12
(too old to reply)
John Paul Adrian Glaubitz
2022-12-26 10:00:01 UTC
Permalink
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
2022-12-26 10:00:01 UTC
Permalink
This fixes the following warning when building with gcc-12 that is
the result of ofdt_path being a fixed-sized array which means that
(char *)ofdt_path never be NULL:

src/drmgr/common_pci.c: In function 'devspec_check_node':
src/drmgr/common_pci.c:465:29: error: the comparison will always evaluate as 'false' for the address of 'ofdt_path' will never be NULL [-Werror=address]
465 | if (node->ofdt_path == NULL)
| ^~
In file included from src/drmgr/drpci.h:25,
from src/drmgr/rtas_calls.h:25,
from src/drmgr/dr.h:30,
from src/drmgr/common_pci.c:31:
src/drmgr/ofdt.h:78:25: note: 'ofdt_path' declared here
78 | char ofdt_path[DR_PATH_MAX];
|

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

diff --git a/src/drmgr/common_pci.c b/src/drmgr/common_pci.c
index cbd48c9..c6dcfdf 100644
--- a/src/drmgr/common_pci.c
+++ b/src/drmgr/common_pci.c
@@ -462,7 +462,7 @@ devspec_check_node(struct dr_node *node, char *sysfs_path,

*found = 0;

- if (node->ofdt_path == NULL)
+ if (!strlen(node->ofdt_path))
return 0;

if (! strcmp(full_of_path, node->ofdt_path)) {
--
2.30.2
Jeffrey Walton
2022-12-26 13:30:01 UTC
Permalink
Hi Adrian,

On Mon, Dec 26, 2022 at 4:55 AM John Paul Adrian Glaubitz
Post by John Paul Adrian Glaubitz
This fixes the following warning when building with gcc-12 that is
the result of ofdt_path being a fixed-sized array which means that
src/drmgr/common_pci.c:465:29: error: the comparison will always evaluate as 'false' for the address of 'ofdt_path' will never be NULL [-Werror=address]
465 | if (node->ofdt_path == NULL)
| ^~
In file included from src/drmgr/drpci.h:25,
from src/drmgr/rtas_calls.h:25,
from src/drmgr/dr.h:30,
src/drmgr/ofdt.h:78:25: note: 'ofdt_path' declared here
78 | char ofdt_path[DR_PATH_MAX];
|
---
src/drmgr/common_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/drmgr/common_pci.c b/src/drmgr/common_pci.c
index cbd48c9..c6dcfdf 100644
--- a/src/drmgr/common_pci.c
+++ b/src/drmgr/common_pci.c
@@ -462,7 +462,7 @@ devspec_check_node(struct dr_node *node, char *sysfs_path,
*found = 0;
- if (node->ofdt_path == NULL)
+ if (!strlen(node->ofdt_path))
return 0;
if (! strcmp(full_of_path, node->ofdt_path)) {
--
2.30.2
Do you need strlen? It looks like you only care that the path is
non-empty. You may be able to save on the call to strlen, and instead,
Post by John Paul Adrian Glaubitz
- if (node->ofdt_path == NULL)
+ if (! node->ofdt_path[0])
return 0;
My apologies if I am mis-parsing things.

Jeff
John Paul Adrian Glaubitz
2022-12-26 16:40:02 UTC
Permalink
Hello Jeffrey!
Post by Jeffrey Walton
Do you need strlen? It looks like you only care that the path is
non-empty. You may be able to save on the call to strlen, and instead,
Post by John Paul Adrian Glaubitz
- if (node->ofdt_path == NULL)
+ if (! node->ofdt_path[0])
return 0;
Using strlen creates a more readable and safer code. And since the string-optimization
routines of the compiler are extremely good these days [1], there is no need trying to
beat the compiler with manual optimizations.

Adrian
Post by Jeffrey Walton
[1]

--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
John Paul Adrian Glaubitz
2022-12-26 10:00:01 UTC
Permalink
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 <***@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
2022-12-26 10:00:01 UTC
Permalink
This fixes the following warning when building with gcc-12 that is
the result of sysfs_dev_path being a fixed-sized array which means
that (char *)sysfs_dev_path never be NULL:

src/drmgr/drslot_chrp_hea.c: In function 'hotplug_port':
src/drmgr/drslot_chrp_hea.c:124:13: error: the comparison will always evaluate as 'true' for the address of 'sysfs_dev_path' will never be NULL [-Werror=address]
124 | if (! hea->sysfs_dev_path) {
| ^
In file included from src/drmgr/drpci.h:25,
from src/drmgr/rtas_calls.h:25,
from src/drmgr/dr.h:30,
from src/drmgr/drslot_chrp_hea.c:31:
src/drmgr/ofdt.h:84:25: note: 'sysfs_dev_path' declared here
84 | char sysfs_dev_path[DR_PATH_MAX];
| ^~~~~~~~~~~~~~

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

diff --git a/src/drmgr/drslot_chrp_hea.c b/src/drmgr/drslot_chrp_hea.c
index 98b96e0..714752a 100644
--- a/src/drmgr/drslot_chrp_hea.c
+++ b/src/drmgr/drslot_chrp_hea.c
@@ -121,7 +121,7 @@ hotplug_port(int action, struct dr_node *hea, struct dr_node *port)

say(DEBUG, "Attempting to hotplug %s Port.\n", action_str);

- if (! hea->sysfs_dev_path) {
+ if (!strlen(hea->sysfs_dev_path)) {
say(DEBUG, "Non-existant sysfs dev path for Port, hotplug "
"failed.\n");
return -EINVAL;
--
2.30.2
John Paul Adrian Glaubitz
2023-01-04 11:40:01 UTC
Permalink
Ping. Any chance we can get these small fixes merged for the next branch?

Thanks,
Adrian
Post by 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
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Nathan Lynch
2023-01-04 17:10:01 UTC
Permalink
Post by John Paul Adrian Glaubitz
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
2023-01-04 23:20:01 UTC
Permalink
Post by Nathan Lynch
Post by John Paul Adrian Glaubitz
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.
Agreed. Thanks Nathan for the immediate turn around on patches to do just that.

-Tyrel
Tyrel Datwyler
2023-01-05 01:00:01 UTC
Permalink
Post by 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
--
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
2023-01-05 09:00:01 UTC
Permalink
Hi Tyrel!
Post by Tyrel Datwyler
[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!

Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
John Paul Adrian Glaubitz
2023-01-05 21:10:01 UTC
Permalink
Hi Tyrel!
Post by 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.
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.

Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Tyrel Datwyler
2023-01-18 00:20:01 UTC
Permalink
Post by John Paul Adrian Glaubitz
Hi Tyrel!
Post by 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.
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.
Post by John Paul Adrian Glaubitz
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.
Post by John Paul Adrian Glaubitz
 
        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
Post by John Paul Adrian Glaubitz
If you're fine with that change, I can post a patch to the mailing list.
Adrian
John Paul Adrian Glaubitz
2023-01-18 09:40:01 UTC
Permalink
Hi Tyrel!
Post by Tyrel Datwyler
Post by John Paul Adrian Glaubitz
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.
Post by John Paul Adrian Glaubitz
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.
Thanks a lot for the elaborate explanation. That helps understanding the problem!
Post by Tyrel Datwyler
Post by John Paul Adrian Glaubitz
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.
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.

Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Tyrel Datwyler
2023-01-05 21:20:01 UTC
Permalink
Post by 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.
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
Post by John Paul Adrian Glaubitz
Cheers,
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Loading...