mbedtls
mbedtls copied to clipboard
Mbed-TLS
Fix compilation when -Werror=maybe-uninitialized is enabled
Description
The now struct in x509_crt.c is not initialized and the compiler complains about it when -Werror=maybe-uninitialized is enabled. This PR solves this by initializing the struct to zero before it is being used.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")
- [x] changelog not required
- [x] backport not required
- [x] tests not required
Notes for the submitter
Please refer to the contributing guidelines, especially the checklist for PR contributors.
Help make review efficient:
- Multiple simple commits
- please structure your PR into a series of small commits, each of which does one thing
- Avoid force-push
- please do not force-push to update your PR - just add new commit(s)
- See our Guidelines for Contributors for more details about the review process.
Hi @daverodgman,
Please, can you take a look to this small PR? This solves an issue when trying to compile in debug mode to do memory inspect.
The error I receive is the following:
Building in debug mode
Compiling .pio/build/TEST/mbedtls/mbedtls/library/x509_crt.o
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c: In function 'x509_crt_verify_chain':
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c:2632:15: error: 'now' may be used uninitialized [-Werror=maybe-uninitialized]
2632 | ret = x509_crt_find_parent(child, cur_trust_ca, &parent,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2633 | &parent_is_trusted, &signature_is_good,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2634 | ver_chain->len - 1, self_cnt, rs_ctx,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2635 | &now);
| ~~~~~
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c:2386:12: note: by argument 9 of type 'const mbedtls_x509_time *' to 'x509_crt_find_parent' declared here
2386 | static int x509_crt_find_parent(
| ^~~~~~~~~~~~~~~~~~~~
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c:2537:23: note: 'now' declared here
2537 | mbedtls_x509_time now;
| ^~~
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c:2684:19: error: 'now' may be used uninitialized [-Werror=maybe-uninitialized]
2684 | *flags |= x509_crt_verifycrl(child, parent, ca_crl, profile, &now);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c:2019:12: note: by argument 5 of type 'const mbedtls_x509_time *' to 'x509_crt_verifycrl' declared here
2019 | static int x509_crt_verifycrl(mbedtls_x509_crt *crt, mbedtls_x509_crt *ca,
| ^~~~~~~~~~~~~~~~~~
/home/ascillato/.platformio/packages/framework-espidf/components/mbedtls/mbedtls/library/x509_crt.c:2537:23: note: 'now' declared here
2537 | mbedtls_x509_time now;
| ^~~
cc1: all warnings being treated as errors
With the proposed change of this PR, this issue is fixed and the compilation runs fine.
The key MBEDTLS_HAVE_TIME_DATE is disabled by default in the SDKCONFIG for ESP32-S3.
Does this need a backport?
I don't know. In that case, how I can help on that backport?
@tom-cosgrove-arm @daverodgman
One of the CI failed but I don't have access to the logs to see why. How I can help with that?
Internal CI: PR tests — Failures: all_u16-build_arm_clang_thumb…
When I click on Details, which goes to https://jenkins-mbedtls.oss.arm.com/job/mbed-tls-pr-head/job/PR-8896-head/3/display/redirect, it gives me:
In that case, how I can help on that backport?
A backport is a version of this PR on the LTS branch(es), currently just mbedtls-2.28. So: is this code there? Is there other code on the tip of mbedtls-2.28 that isn't -Werror=maybe-uninitialized-clean?
One of the CI failed but I don't have access to the logs to see why. How I can help with that?
Apologies, I meant to run the CI on the public OpenCI, but typed internal by mistake. I've started on the OpenCI (it would be needed anyway for approval) but several failures, for example
[2024-03-06T20:13:56.914Z] CC x509_crt.c
[2024-03-06T20:13:57.194Z] x509_crt.c:2526:33: error: missing field 'mon' initializer [-Werror,-Wmissing-field-initializers]
[2024-03-06T20:13:57.194Z] mbedtls_x509_time now = { 0 };
[2024-03-06T20:13:57.194Z] ^
[2024-03-06T20:13:57.194Z] 1 error generated.
:
Ÿ[2024-03-06T20:13:59.331Z] FAILED: 1 components
[2024-03-06T20:13:59.331Z] build_psa_accel_alg_ecdh: build: full - MBEDTLS_USE_PSA_CRYPTO + PSA_WANT_ALG_ECDH without MBEDTLS_ECDH_C: make CC=$ASAN_CC CFLAGS="$ASAN_CFLAGS -DPSA_CRYPTO_DRIVER_TEST -DMBEDTLS_PSA_ACCEL_ALG_ECDH -I../tests/include" LDFLAGS="$ASAN_CFLAGS" -> 2
Same failure in another component with
[2024-03-06T20:24:36.861Z] -- The C compiler identification is Clang 3.8.0
ran into same issue and found this PR.. ..
so, change that is permissible by language standard is not accepted because hey we have our ci from 90's and we won't change it for whatever reason ....
making the life difficult for contributors will eventually move away any good contributions
why can't the ci be fixed here? why the author needs to revisit this simple fix multiple time?
why can't the ci be fixed here?
We aim to support as large a range of C compilers as possible, and we know that some of our users do use older systems (as we get complaints in the other direction too). This is "business as usual" for source code libraries that target a wide range of systems.
A suggestion was made that would satisfy both, but hasn't (yet/) been acted on