hmac.c compilation error with gcc-11.1 new warning->error pointer vs array

[saulwold]

With the newer gcc-11.1 compiler and warning set to error, the following error occurs:

/git/src/hmac.c:211:43: error: argument 2 of type 'uint8_t *' {aka 'unsigned char *'} declared as a pointer [-Werror=array-parameter=] 211 | int hmacResult(HMACContext *ctx, uint8_t *digest) | ~~~~~~~~~^~~~~~ In file included from /git/src/hmac.c:13: /git/inc/azure_c_shared_utility/sha.h:252:42: note: previously declared as an array 'uint8_t[64]' {aka 'unsigned char[64]'} 252 | int hmacResult(HMACContext *ctx, uint8_t digest[USHAMaxHashSize]); | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors I have attached a simple patch to address this and could sumbit a pull request instead. Sau!

[arsing]

I don't see a patch in the OP, but since I hit the same problem myself:

diff --git a/src/hmac.c b/src/hmac.c
index 53f2411f..160af9d2 100644
--- a/src/hmac.c
+++ b/src/hmac.c
@@ -208,7 +208,7 @@ int hmacFinalBits(HMACContext *ctx,
 *   sha Error Code.
 *
 */
-int hmacResult(HMACContext *ctx, uint8_t *digest)
+int hmacResult(HMACContext *ctx, uint8_t digest[USHAMaxHashSize])
 {
     if (!ctx) return shaNull;

However it also needs umock-c changes, because of multiple cases of:

inc/azure_c_shared_utility/crt_abstractions.h: In function ‘unsignedIntToString’:
deps/umock-c/inc/umock_c/umock_c_internal.h:145:15: error: array subscript ‘void *[0]’ is partly outside array bounds of ‘unsigned int[1]’ [-Werror=array-bounds]
  145 |         (void)memcpy(*((void**)(&arg_name)), matched_call_data->out_arg_buffers[COUNT_OF(matched_call_data->out_arg_buffers) - MU_DIV2(count)]->bytes, matched_call_data->out_arg_buffers[COUNT_OF(matched_call_data->out_arg_buffers) - MU_DIV2(count)]->length); \

[danewalton]

I believe this is fixed with #537. Closing for now.

[arsing]

@danewalton What about the error in my comment?

[danewalton]

Sorry @arsing I think I was looking at the cached issue page and didn't see your comment.

Can you give more details on what/how you're building? I'm not seeing that error building on windows for unit tests.

[saulwold]

@danewalton the issue is building on a Linux or other target using the new gcc-11 compiler. You might not see it on windows.

[danewalton]

ahh yup as the title of the issue says my apologies. I'll check in on this and get back here. Reopening until then.

[danewalton]

Hey @arsing with this PR I'm seeing a successful build using gcc11.1. Can you confirm? https://github.com/Azure/azure-c-shared-utility/pull/552

[arsing]

The error I reported in my comment has not been fixed. As I said it's in your umock-c dependency, so you need to fix it there first and then update the submodule dep here.

You can see the error yourself (along with some others in your own tests related to sprintf not using a large enough buffer) with:

cmake .. -Drun_unittests:bool=ON -DCMAKE_BUILD_TYPE=Release
cmake --build . -j

[danewalton]

Have you tried pulling the branch from the PR and updating submodules? Here is my build output with commands that were run. We might have done a submodule dance update which fixed it.

output.txt

[arsing]

I wrote -Drun_unittests:bool=ON -DCMAKE_BUILD_TYPE=Release whereas you ran with just -Drun_unittests:bool=ON.

(There's no need to guess whether the submodule was updated or not. https://github.com/Azure/azure-c-shared-utility/tree/master/deps - the umock-c was last updated 2 years ago.)

[ericwolz]

Can you explain why you are building the unit tests on? The expectation is that this is only for internal use build validation.

[arsing]

To simulate the failure of umock-c (pulled in via azure-c-shared-utility) in our (iotedge) repo's tests, without giving you a whole new cmake project that you need to repro. Your own tests repro the failure if they're compiled in release mode.

(Why do we compile our tests in release mode? Because our code is written in Rust so the compiled binary always links to the release MSVCRT, and it's not allowed for release and debug MSVCRT to be mixed in the same process.)

[danewalton]

It looks like there are several people that have been bit by this warning popping up in GCC 11.1.

GCC Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124 StackOverflow: https://stackoverflow.com/questions/67584073/gcc-11-false-array-subscript-is-partly-outside-array-bounds-warning

GCC 11.2 looks to have resolved those false warnings

I have confirmed a clean build using GCC 11.2. Going to consider this closed then.

[arsing]

I see the errors I mentioned with (what my distro calls) gcc 11.2.1

[ericwolz]

Thanks. We will advise customers to stay with GCC 11.2.

[arsing]

That isn't a thing. Distributions don't carry multiple versions of gcc 11.

[danewalton]

I stand corrected it still has the issue my CMakeCache.txt somehow stuck in between builds. Even if that's the case, I don't see an issue with the code and would say, unless proven otherwise, this is falsely popping up as others have stated. I'd rather not change code to try and hack our way around a false flag while GCC figures their warning out. For now I would recommend if people want to build our repo with the following conditions:

• GCC 11.1 +
• Release mode
• Unit tests

To compile with -Wno-array-bounds.

[arsing]

To be clear, the conditions are:

• GCC 11
• Release mode

[danewalton]

Ah thanks for the clarification. With this PR I'm getting a clean build using GCC 11.2 with the compile flag -Wno-array-bounds. https://github.com/Azure/azure-c-shared-utility/pull/553

[danewalton]

For those that need to compile under these conditions, you can manually add the flag to this section of the build flags. https://github.com/Azure/azure-c-shared-utility/blob/92cc8891c51308e039dd9c46b5fb8f3a4544ccb8/configs/azure_iot_build_rules.cmake#L74-L75

[arsing]

Okay, if you don't want to fix umock-c we can disable the warning when compiling our tests.

Do you know when you'll make a new release of c-shared-utility with the hmac.c fix?

[arsing]

BTW, re:

Even if that's the case, I don't see an issue with the code and would say, unless proven otherwise, this is falsely popping up as others have stated. I'd rather not change code to try and hack our way around a false flag while GCC figures their warning out.

I'm not sure error: array subscript ‘void *[0]’ is partly outside array bounds of ‘unsigned int[1]’ isn't a bug. It sounds like the code memcpy'ing one void* worth of data (64-bit on an x86_64 glibc target) from an unsigned int[1] (32-bit on an x86_64 glibc target).

The GCC bug you linked to, while about the same warning, was not only a different issue (the index was the same and there was no type punning involved) but also was fixed in GCC 8. I don't think the warning is quite in need of being "figured out" as you say.

Edit: (But to be clear I'm not concerned about this. For us it's just test code.)

[danewalton]

Thank you for the comments I'm all for making things right (and learning here if I'm wrong). In this case, I see from memcpy docs:

void *memcpy(void *dest, const void *src, size_t count);

memcpy copies count bytes from src to dest

"Bytes" emphasis mine.

In this case, we have the function call:

(void)memcpy(*((void**)(&arg_name)),
matched_call_data->out_arg_buffers[COUNT_OF(matched_call_data->out_arg_buffers) - MU_DIV2(count)]->bytes,
matched_call_data->out_arg_buffers[COUNT_OF(matched_call_data->out_arg_buffers) - MU_DIV2(count)]->length);

So ->length bytes from ->bytes will be deposited in *((void**)&arg_name), not necessarily one void* worth of data from ->bytes.

[arsing]

Do you know when you'll make a new release of c-shared-utility with the hmac.c fix?

[ericwolz]

There are no releases or SLA on the usage of this repo. SLA and releases only applies to the SDKs that directly depend on this repo.