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 "inconsistent annotation" warnings #7531

Open
irwir opened this issue Apr 30, 2023 · 7 comments
Open

Fix "inconsistent annotation" warnings #7531

irwir opened this issue Apr 30, 2023 · 7 comments
Labels
component-tls enhancement good-first-issue Good for newcomers help-wanted This issue is not being actively worked on, but PRs welcome.

Comments

@irwir
Copy link
Contributor

irwir commented Apr 30, 2023

Suggested enhancement

All functions declared as MBEDTLS_CHECK_RETURN_CRITICAL should have this annotation in definitions.

Justification

Currently some functions are declared in ssl_misc.h and defined in ssl_tls.c with MBEDTLS_CHECK_RETURN_CRITICAL annotation.
For unknown reason there are definitions with omitted annotation, and this gives warnings in VS 2022.
The warning is minor, but for consistency and quiet compilation this should be amended.

@tom-daubney-arm tom-daubney-arm self-assigned this May 2, 2023
@tom-daubney-arm
Copy link
Contributor

tom-daubney-arm commented May 2, 2023

Hi, thanks for the report.

Some specific examples of the functions you describe would be really helpful. I would invite you to raise a PR against the respository which implements the fix you want to see. This would be the fastest way to close the issue. Would you have time to do this? In the meantime I will have a look and try to replicate the isue, although I am not a Windows user. Thanks

@irwir
Copy link
Contributor Author

irwir commented May 2, 2023

int mbedtls_ssl_session_copy(mbedtls_ssl_session *dst,
int mbedtls_ssl_add_hs_hdr_to_checksum(mbedtls_ssl_context *ssl,
int mbedtls_ssl_add_hs_hdr_to_checksum(mbedtls_ssl_context *ssl,
int mbedtls_ssl_add_hs_msg_to_checksum(mbedtls_ssl_context *ssl,
int mbedtls_ssl_reset_checksum(mbedtls_ssl_context *ssl)
int mbedtls_ssl_session_reset_int(mbedtls_ssl_context *ssl, int partial)
int mbedtls_ssl_conf_has_static_psk(mbedtls_ssl_config const *conf)
int mbedtls_ssl_start_renegotiation(mbedtls_ssl_context *ssl)
int mbedtls_ssl_check_curve_tls_id(const mbedtls_ssl_context *ssl, uint16_t tls_id)
int mbedtls_ssl_check_curve(const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id)
int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert,
int mbedtls_ssl_get_handshake_transcript(mbedtls_ssl_context *ssl,
int mbedtls_ssl_parse_sig_alg_ext(mbedtls_ssl_context *ssl,
int mbedtls_ssl_derive_keys(mbedtls_ssl_context *ssl)
int mbedtls_ssl_set_calc_verify_md(mbedtls_ssl_context *ssl, int md)
int mbedtls_ssl_psk_derive_premaster(mbedtls_ssl_context *ssl, mbedtls_key_exchange_type_t key_ex)
int mbedtls_ssl_write_certificate(mbedtls_ssl_context *ssl)
int mbedtls_ssl_parse_certificate(mbedtls_ssl_context *ssl)
int mbedtls_ssl_write_finished(mbedtls_ssl_context *ssl)
int mbedtls_ssl_parse_finished(mbedtls_ssl_context *ssl)
int mbedtls_ssl_get_key_exchange_md_tls1_2(mbedtls_ssl_context *ssl,
int mbedtls_ssl_validate_ciphersuite(
int mbedtls_ssl_write_sig_alg_ext(mbedtls_ssl_context *ssl, unsigned char *buf,
int mbedtls_ssl_write_alpn_ext(mbedtls_ssl_context *ssl,

Hope it helps.

@tom-daubney-arm
Copy link
Contributor

For further information, are there any specific build options you use when these warnings appear or is it a case of any and all builds will have these warnings?

@irwir
Copy link
Contributor Author

irwir commented May 2, 2023

Probably it was this one: https://learn.microsoft.com/en-us/cpp/code-quality/c28252?view=msvc-170
Then code analysis should be enabled.

@tom-daubney-arm
Copy link
Contributor

tom-daubney-arm commented May 2, 2023

Sorry I mean are you passing any specific build options etc to the compiler that trigger the warnings? Or are you jsut building with everything default and seeing them?

e: I didn't realise you had to manually enable some warnings in VS, now I get why you sent the link. Thanks for that.

@tom-daubney-arm
Copy link
Contributor

tom-daubney-arm commented May 2, 2023

Just to update: We have tried to replicate these warnings on VS 2022 but have as yet been unable to do so, so we are going to need more information about the steps needed to reproduce the issue.

Please may we have your full reproduction steps, including things like retarget options, system evironment etc. The mbedtls version you are using would be good too. The more you can tell us the better. Thanks

@tom-daubney-arm
Copy link
Contributor

We have now verified the existence of these warnings. Fixing this would be an improvement but it is not something we can prioritise at present. We would welcome a PR from the community for this. Also, it should be noted that the full fix for this would include having something in the CI to recreate the VS build that shows these warnings.

@tom-daubney-arm tom-daubney-arm added help-wanted This issue is not being actively worked on, but PRs welcome. good-first-issue Good for newcomers enhancement labels May 2, 2023
@tom-daubney-arm tom-daubney-arm removed their assignment May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls enhancement good-first-issue Good for newcomers help-wanted This issue is not being actively worked on, but PRs welcome.
Projects
None yet
Development

No branches or pull requests

2 participants