Skip to content

Fix MSVC build issue from MbedTls issue 7087 #258

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

ccrugoPhilips
Copy link

@ccrugoPhilips ccrugoPhilips commented Apr 15, 2025

Description

Refactored code causing build issues with MSVC toolset v142 and prior. This has been tracked under MbedTls issue 7087.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog provided | not required because:
  • framework PR provided Mbed-TLS/mbedtls-framework# | not required
  • mbedtls development PR provided Mbed-TLS/mbedtls# | not required because:
  • mbedtls 3.6 PR provided Mbed-TLS/mbedtls# | not required because:
  • tests provided | not required because:

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.

Copy link
Contributor

@ronald-cron-arm ronald-cron-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 for this contribution. We are not comfortable with the changes in crypto.h. Please see my comment below.

*/
static psa_key_attributes_t psa_key_attributes_init(void);
static void psa_key_attributes_init_internal(psa_key_attributes_t *attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

This defines a new API and we cannot really do that.

In #260 I have implemented another solution experimented in Mbed-TLS/mbedtls#7087 (see Mbed-TLS/mbedtls#7087 (comment)) we would be more comfortable with.

I've also worked on crypto_extra.h in #260 and looking at your code it seems we ended up with the same changes. I am happy to remove crypto_extra.h changes from #260 if you prefer.

Copy link
Author

@ccrugoPhilips ccrugoPhilips Apr 18, 2025

Choose a reason for hiding this comment

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

Moving the include is indeed much simpler to fix build errors from crypto.h and crypto_struct.h; I'm not very familiar with the API so tried to avoid that. Will rollback my changes in those files.

I see in #260 that some non-regression testing for Mbed-TLS/mbedtls#7087 may be needed, should that be added in this PR? Would be nice to keep crypto_extra.h changes in this PR if you don't mind.

@ccrugoPhilips
Copy link
Author

A couple of additional remarks:

The build issues reported in Mbed-TLS/mbedtls#7087 are not present anymore with VS2022 (MSVC toolset v143).

Added an extra fix for a warning being treated as error when MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled.

@ccrugoPhilips ccrugoPhilips force-pushed the fix/vs2019_build_issue_7087 branch from fb49fd7 to 7a96523 Compare April 18, 2025 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants