Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build failure on gcc-11 #3848

Merged
merged 7 commits into from Dec 1, 2020

Conversation

rodrigo-dc
Copy link
Contributor

@rodrigo-dc rodrigo-dc commented Nov 4, 2020

Function prototypes changed to use array parameters instead of
pointers.

Signed-off-by: Rodrigo Dias Corrêa rodrigo@correas.us

Description

Fixes #3782

Status

READY

Requires Backporting

I don't know.

Migrations

NO

@yanesca yanesca added Community needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Nov 4, 2020
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog entry file in ChangeLog.d indicating that this fixes the build with GCC 11 and referencing #3782.

library/ssl_tls.c Outdated Show resolved Hide resolved
OBSOLETE - PLEASE SEE https://github.com/orgs/Mbed-TLS/projects/2 automation moved this from To Do to In Progress Nov 9, 2020
@gilles-peskine-arm gilles-peskine-arm added component-platform Portability layer and build scripts needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Nov 9, 2020
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm happy with the PR now except that I'd rather avoid wasting stack space for the sake of a compiler warning.

library/ssl_tls.c Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Nov 19, 2020
@gilles-peskine-arm
Copy link
Contributor

LTS branches should work under newer compilers too, so once we agree on the approach, this will need backports to mbedtls-2.16 and mbedtls-2.7.

Function prototypes changed to use array parameters instead of
pointers.

Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
In GCC 11, parameters declared as arrays in function prototypes
cannot be declared as pointers in the function definition. The
same is true for the other way around.

The definition of `mbedtls_aes_cmac_prf_128` was changed to match
its public prototype in `cmac.h`. The type `output` was
`unsigned char *`, now is `unsigned char [16]`.

In `ssl_tls.c`, all the `ssl_calc_verify_*` variants now use pointers
for the output `hash` parameter. The array parameters were removed
because those functions must be compatible with the function pointer
`calc_verify` (defined in `ssl_internal.h`).

Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
GCC 11 generated the warnings because the parameter `ret_buf`
was declared as `const char[10]`, but some of the arguments
provided in `run_test_snprintf` are shorter literals, like "".

Now the type of `ret_buf` is `const char *`.
Both implementations of `test_snprintf` were fixed.

Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
GCC 11 generated a warning because `padbuf` was too small to be
used as an argument for `mbedtls_sha512_finish_ret`. The `output`
parameter of `mbedtls_sha512_finish_ret` has the type
`unsigned char[64]`, but `padbuf` was only 48 bytes long.

Even though `ssl_calc_finished_tls_sha384` uses only 48 bytes for
the hash output, the size of `padbuf` was increased to 64 bytes.

Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
This commit fixes the same warning fixed by baeedbf, but without
wasting RAM. By casting `mbedtls_sha512_finish_ret()`, `padbuf`
could be kept 48 bytes long without triggering any warnings.

Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
`finish_sha384_t` was made more generic by using `unsigned char*`
instead of `unsigned char[48]` as the second parameter.
This change tries to make the function casting more robust against
future improvements of gcc analysis.

Signed-off-by: Rodrigo Dias Correa <rodrigo@correas.us>
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

This needs to be backported to 2.16 and 2.7 (where applicable). Please wait for a second approval on this PR.

Copy link
Contributor

@chris-jones-arm chris-jones-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. As already mentioned, this just needs backports and then we can merge.

@chris-jones-arm chris-jones-arm removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Nov 26, 2020
@rodrigo-dc
Copy link
Contributor Author

Looks good to me. As already mentioned, this just needs backports and then we can merge.

Ok. Backported: #3925 and #3926.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts
Projects
None yet
5 participants