Skip to content

Conversation

@bensze01
Copy link
Contributor

@bensze01 bensze01 commented Feb 28, 2025

Description

This PR fixes the preprocessor guards protecting the use of %zu in MBEDTLS_PRINTF_SIZET
It also adds a non-regression test to test_suite_debug.

Fixes #10017
Unblocks #9976

PR checklist

@bensze01 bensze01 added bug needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-tls needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Feb 28, 2025
@gilles-peskine-arm gilles-peskine-arm requested review from minosgalanakis and removed request for mpg February 28, 2025 22:37
@bensze01 bensze01 force-pushed the msvc-format-size-macros branch 4 times, most recently from a8f4a16 to 7f1dfaa Compare March 2, 2025 00:38
@bensze01 bensze01 added the needs-preceding-pr Requires another PR to be merged first label Mar 2, 2025
@bensze01 bensze01 force-pushed the msvc-format-size-macros branch 4 times, most recently from 37793b4 to 5fbffa2 Compare March 3, 2025 12:29
@mpg
Copy link
Contributor

mpg commented Mar 5, 2025

I tried cherry-picking 81ea6a6 in the branch (based on 3.6) where I originally uncovered the issue. It fixed the windows -cmake tests but broke the the windows -shipped tests with lots of compile errors like: error C2146: syntax error : missing ')' before identifier 'PRIuPTR' (full log).

I can see that the windows -shipped tests are passing here, so perhaps I didn't cherry-pick correctly, or the problem I'm observing is specific to 3.6?

(Unrelated: this is labeled "needs preceding PR" but I can't see a link to the preceding PR it needs in the description.)

@bensze01
Copy link
Contributor Author

bensze01 commented Mar 5, 2025

@mpg Both your issues and the 'Needs preceding pr' are because one of the commits (5fbffa2) updates the tf-psa-crypto submodule, which I haven't filed a PR for yet. You need to cherry-pick that one as well.

@mpg
Copy link
Contributor

mpg commented Mar 5, 2025

Ok, thanks.

@mpg
Copy link
Contributor

mpg commented Mar 7, 2025

@bensze01 How is this progressing? This is needed for the TLS defragmentation work which is the top priority for 3.6.3 and the code freeze is next Friday (14 Mar) - hard deadline.

If this can't be ready in time we can look for workarounds but we need to know.

@bensze01
Copy link
Contributor Author

bensze01 commented Mar 7, 2025

@mpg Sorry for the delay - it should be ready for review now. Will post the TF-PSA-Crypto pr in a moment.

@bensze01 bensze01 force-pushed the msvc-format-size-macros branch 2 times, most recently from 28a9276 to d656263 Compare March 7, 2025 16:40
The Windows CRT treats any invalid format specifiers passed to the CRT
as fatal assertion failures. Disable thie behaviour temporarily while
testing if the format specifiers we use are supported.

Signed-off-by: Bence Szépkúti <[email protected]>
Visual Studio 2013 (_MSC_VER == 1800) doesn't support %zu - only use it
on 2015 and above (_MSC_VER >= 1900).

%ldd works on Visual Studio 2013, but this patch keeps the two macro
definitions together, for simplicity's sake.

Signed-off-by: Bence Szépkúti <[email protected]>
These headers were necessary for compatibility with Visual Studio 2010,
and interfere with the system headers on Visual Studio 2013+, eg. when
building Mbed TLS using the .sln file shipped with the project.

Move the still-required definition of "inline" to callconv.h, where the
definition for GCC also lives.

Signed-off-by: Bence Szépkúti <[email protected]>
Signed-off-by: Bence Szépkúti <[email protected]>
Signed-off-by: Bence Szépkúti <[email protected]>
Signed-off-by: Bence Szépkúti <[email protected]>
Use a dummy definition of mbedtls_ms_time_t in builds without
MBEDTLS_HAVE_TIME.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01 bensze01 force-pushed the msvc-format-size-macros branch from 2a63cac to d2933f0 Compare March 12, 2025 16:13
@bensze01 bensze01 added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-work labels Mar 12, 2025
@bensze01 bensze01 force-pushed the msvc-format-size-macros branch from d2933f0 to d86ad8f Compare March 12, 2025 16:41
@bensze01 bensze01 force-pushed the msvc-format-size-macros branch from d86ad8f to 24f11a3 Compare March 12, 2025 18:14
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 for simplifying the test data. LGTM

@minosgalanakis minosgalanakis added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Mar 13, 2025
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@github-project-automation github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Mar 13, 2025
@bensze01 bensze01 added this pull request to the merge queue Mar 13, 2025
Merged via the queue into Mbed-TLS:development with commit 906d3cd Mar 13, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Mar 13, 2025
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 bug component-tls priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Development

Successfully merging this pull request may close these issues.

Wrong MSVC version guard for C99 format size specifiers

4 participants