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
>
> If you're fine with that change, I can post a patch to the mailing list.
>
> Adrian
>
