gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Add winsock2 header into build_info.h #262

Open
irwir wants to merge 3 commits from irwir/winsock2 into development
pull from: irwir/winsock2
merge into: gh-Mbed-TLS:development
Conversation 25 Commits 3 Files changed 2 +34 -27
irwir commented 2025-04-19 06:55:49 -05:00 (Migrated from github.com)

Description

Ensures proper socket definitions on Windows (as a part of resolving mbedtls issue #10097).

PR checklist

  • changelog not required
  • framework PR provided #193
  • mbedtls development PR provided #10140
  • mbedtls 3.6 PR not required because: not planned
  • tests not required
## Description Ensures proper socket definitions on Windows (as a part of resolving mbedtls issue #10097). ## PR checklist - [x] **changelog** not required - [x] **framework PR** provided [#193](https://github.com/Mbed-TLS/mbedtls-framework/pull/193) - [x] **mbedtls development PR** provided [#10140](https://github.com/Mbed-TLS/mbedtls/pull/10140) - [x] **mbedtls 3.6 PR** not required because: not planned - **tests** not required
gilles-peskine-arm commented 2025-04-22 03:29:22 -05:00 (Migrated from github.com)

TF-PSA-Crypto needs to build outside of Mbed TLS. So it doesn't seem right to me that the Windows build (with an unchanged mbedtls) is failing here.

TF-PSA-Crypto needs to build outside of Mbed TLS. So it doesn't seem right to me that the Windows build (with an unchanged mbedtls) is failing here.
irwir commented 2025-04-22 05:00:49 -05:00 (Migrated from github.com)

This is odd if an additional standard header breaks things so badly.

[2025-04-21T19:59:35.604Z] 291>C:/Users/Administrator/AppData/Local/Temp/tmp5cfern6y/tf-psa-crypto/tests/suites/test_suite_ecp.function(19): error C2143: syntax error : missing ')' before 'constant' [C:\Users\Administrator\AppData\Local\Temp\tmp5cfern6y\cmake_solution\tf-psa-crypto\tests\test_suite_ecp.vcxproj]

How to parse this?
The C2143 error code from a compiler and the last word vcxproj indicate a compilation error.
But the source file is test_suite_ecp.function while make build uses tf-psa-crypto/tests/test_suite_ecp.c?

This is odd if an additional standard header breaks things so badly. > [2025-04-21T19:59:35.604Z] 291>C:/Users/Administrator/AppData/Local/Temp/tmp5cfern6y/tf-psa-crypto/tests/suites/test_suite_ecp.function(19): error C2143: syntax error : missing ')' before 'constant' [C:\Users\Administrator\AppData\Local\Temp\tmp5cfern6y\cmake_solution\tf-psa-crypto\tests\test_suite_ecp.vcxproj] How to parse this? The C2143 error code from a compiler and the last word `vcxproj` indicate a compilation error. But the source file is `test_suite_ecp.function` while _make_ build uses `tf-psa-crypto/tests/test_suite_ecp.c`?
irwir commented 2025-04-22 05:05:29 -05:00 (Migrated from github.com)

[2025-04-21T19:59:36.764Z] cl : Command line warning D9002: ignoring unknown option '/utf-8' [C:\Users\Administrator\AppData\Local\Temp\tmp5cfern6y\cmake_solution\tf-psa-crypto\tests\test_suite_gcm.aes128_en.vcxproj]

Numerous warnings in the log. Maybe VS 2013 did not have this option?
Unlike for mbdetls, tests were never tried with later versions of Visual Studio.

> [2025-04-21T19:59:36.764Z] cl : Command line warning D9002: ignoring unknown option '/utf-8' [C:\Users\Administrator\AppData\Local\Temp\tmp5cfern6y\cmake_solution\tf-psa-crypto\tests\test_suite_gcm.aes128_en.vcxproj] Numerous warnings in the log. Maybe VS 2013 did not have this option? Unlike for `mbdetls`, tests were never tried with later versions of Visual Studio.
gilles-peskine-arm commented 2025-04-22 07:41:17 -05:00 (Migrated from github.com)

test_suite_xxx.c is generated from test_suite_xxx.function using framework/scripts/generate_test_code.py. It does a couple of substitutions and adds some boilerplate as well as code to prepare arguments to test functions.

The offending line (more visible in the message from MinGW) is

inline static int mbedtls_ecp_group_cmp(mbedtls_ecp_group *grp1,

It looks like the parser gets confused because it doesn't recognize inline as intended.

We use the inline keyword in our code base. For compilers such as MSVC that don't define it, tf-psa-crypto/build_info.h defines it. This happens shortly before the inclusion of winsock2.h. I have no idea why including winsock2.h breaks this.

As for /utf-8, the warning is annoying but harmless. It was added to fix a warning that does break the build in some environments (those using a non-UTF-8 multibyte encoding by default, if I understand correctly).

`test_suite_xxx.c` is generated from `test_suite_xxx.function` using `framework/scripts/generate_test_code.py`. It does a couple of substitutions and adds some boilerplate as well as code to prepare arguments to test functions. The offending line (more visible in the message from MinGW) is ``` inline static int mbedtls_ecp_group_cmp(mbedtls_ecp_group *grp1, ``` It looks like the parser gets confused because it doesn't recognize `inline` as intended. We use the `inline` keyword in our code base. For compilers such as MSVC that don't define it, [`tf-psa-crypto/build_info.h`](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/include/tf-psa-crypto/build_info.h#L75) defines it. This happens shortly before the inclusion of `winsock2.h`. I have no idea why including `winsock2.h` breaks this. As for `/utf-8`, the warning is annoying but harmless. It was [added](https://github.com/Mbed-TLS/mbedtls/pull/5365) to fix a [warning that does break the build in some environments](https://github.com/Mbed-TLS/mbedtls/issues/4205) (those using a non-UTF-8 multibyte encoding by default, if I understand correctly).
irwir commented 2025-04-22 10:27:51 -05:00 (Migrated from github.com)

test_suite_xxx.c is generated from test_suite_xxx.function

The question was why .function instead of the expected .c in the compiler diagnostics. This was explained by #line directive.

I have no idea why including winsock2.h breaks this.

It is difficult to guess without installed VS 2013 and the generated projects and sources.
A trivial inlined function after including build_info.h could be compiled even in VS 2012. The issue should be somewhere deeper.
With VS 2022, I copied and renamed cert_app.vcxproj, changed the source file to test_suite_ecp.c. Some paths had to be added, but finally it compiled.

> `test_suite_xxx.c` is generated from `test_suite_xxx.function` The question was why `.function` instead of the expected `.c` in the compiler diagnostics. This was explained by #line directive. > I have no idea why including `winsock2.h` breaks this. It is difficult to guess without installed VS 2013 and the generated projects and sources. A trivial inlined function after including `build_info.h` could be compiled even in VS 2012. The issue should be somewhere deeper. With VS 2022, I copied and renamed cert_app.vcxproj, changed the source file to test_suite_ecp.c. Some paths had to be added, but finally it compiled.
irwir commented 2025-04-22 12:29:56 -05:00 (Migrated from github.com)

As for /utf-8

Available since VS 2015.

> As for `/utf-8` Available since VS 2015.
irwir commented 2025-04-23 07:39:13 -05:00 (Migrated from github.com)

As far as I remember, a bug in VS 2015 increased the minimum required version for mbedtls to VS 2017, even though the official documentation still says VS 2013 (also in solution path visualc/VS2013/mbedTLS.sln).

Quote from the current README.md in TF-PSA-Crypto:

The following instructions have been tested on Microsoft Visual Studio Community 2019 Version 16.11.26.

Yet there is nothing but VS 2013 in TF OpenCI: PR tests.

Should this have been changed in development branches?

As far as I remember, a bug in VS 2015 increased the minimum required version for `mbedtls` to VS 2017, even though the official documentation still says VS 2013 (also in solution path `visualc/VS2013/mbedTLS.sln`). Quote from the current `README.md` in `TF-PSA-Crypto`: > The following instructions have been tested on Microsoft Visual Studio Community 2019 Version 16.11.26. Yet there is nothing but VS 2013 in `TF OpenCI: PR tests`. Should this have been changed in development branches?
irwir commented 2025-05-15 05:36:18 -05:00 (Migrated from github.com)

Two questions.
Could TF OpenCI: PR tests be retried with VS 2017?
Would it be possible to get the failing generated projects and source files, as artifacts maybe?
At least one, like test_suite_ecp.c and test_suite_ecp.vcxproj for investigation in VS 2017.

It looks like the parser gets confused because it doesn't recognize inline as intended.

Text search in TF-PSA-Crypto sources had found 51 files containing the inline word. A few tests had been built successfully before the failure.

Two questions. Could TF OpenCI: PR tests be retried with VS 2017? Would it be possible to get the failing generated projects and source files, as artifacts maybe? At least one, like _test_suite_ecp.c_ and _test_suite_ecp.vcxproj_ for investigation in VS 2017. > It looks like the parser gets confused because it doesn't recognize `inline` as intended. Text search in _TF-PSA-Crypto_ sources had found 51 files containing the `inline` word. A few tests had been built successfully before the failure.
gilles-peskine-arm commented 2025-05-15 05:46:43 -05:00 (Migrated from github.com)

Could TF OpenCI: PR tests be retried with VS 2017?

Sure. Since the last CI run, we've switched our CI from VS2013 to VS2017. I've kicked a new run.

Would it be possible to get the failing generated projects and source files, as artifacts maybe?

Setting this up would require some Groovy code. But I don't really see the point: these files don't depend on the platform, on the library configuration or (in principle) on tool versions, you can generate them locally on any platform.

> Could TF OpenCI: PR tests be retried with VS 2017? Sure. Since the last CI run, we've switched our CI from VS2013 to VS2017. I've kicked a new run. > Would it be possible to get the failing generated projects and source files, as artifacts maybe? Setting this up would require some Groovy code. But I don't really see the point: these files don't depend on the platform, on the library configuration or (in principle) on tool versions, you can generate them locally on any platform.
irwir commented 2025-05-16 06:42:49 -05:00 (Migrated from github.com)

Setting this up would require some Groovy code. But I don't really see the point: these files don't depend on the platform, on the library configuration or (in principle) on tool versions, you can generate them locally on any platform.

It could be generated provided everything is installed, configured and it is perfectly well known what commands to run.
But there is only VS 2017 for C/C++ builds. Nothing else, and nothing is planned to be added.
There is also Ubuntu 25.04 which is used for generating files with make.
So it was cheaper to manually create a VS project for the test code and then figure out what was wrong.

Build process now is working, but tests are failing.
With make all two versions had been built - with included winsock2.h and without it.
The only difference in the sources and generated files was build_info.h.
Knowing this, I locally created two builds of test_suite_ssl - with winsock2.h and with the #include commented out.
All compiled files were deleted before the rebuild.
The results are exactly the same, some of Session serialization tests had failed.

> Setting this up would require some Groovy code. But I don't really see the point: these files don't depend on the platform, on the library configuration or (in principle) on tool versions, you can generate them locally on any platform. It could be generated provided everything is installed, configured and it is perfectly well known what commands to run. But there is only VS 2017 for C/C++ builds. Nothing else, and nothing is planned to be added. There is also Ubuntu 25.04 which is used for generating files with make. So it was cheaper to manually create a VS project for the test code and then figure out what was wrong. Build process now is working, but tests are failing. With `make all` two versions had been built - with included winsock2.h and without it. The only difference in the sources and generated files was `build_info.h`. Knowing this, I locally created two builds of `test_suite_ssl` - with `winsock2.h` and with the `#include` commented out. All compiled files were deleted before the rebuild. The results are exactly the same, some of _Session serialization_ tests had failed.
irwir commented 2025-05-16 12:51:58 -05:00 (Migrated from github.com)

Every test in test_suite_ssl.datax that contains path name of a certificate file - failed.
Corrected path names had this outcome:
PASSED (854 / 854 tests (146 skipped))

Every test in `test_suite_ssl.datax` that contains path name of a certificate file - failed. Corrected path names had this outcome: `PASSED (854 / 854 tests (146 skipped))`
gilles-peskine-arm commented 2025-05-16 15:52:53 -05:00 (Migrated from github.com)

You need to run the tests suites from the tests directory.

There's code in tests/suites/host_test.function that tries to automatically change into the directory containing the executable. This is relatively new but our scripts might have gotten used to that. But that code isn't implemented for Windows (patches welcome).

You need to run the tests suites from the `tests` directory. There's code in `tests/suites/host_test.function` that tries to automatically change into the directory containing the executable. This is relatively new but our scripts might have gotten used to that. But that code isn't implemented for Windows (patches welcome).
irwir commented 2025-05-17 04:57:26 -05:00 (Migrated from github.com)

github.com/Mbed-TLS/TF-PSA-Crypto@864dee0530/tests/suites/host_test.function (L446-L449)
This code correctly changed directory for me; verified with _getcwd().
Certainly backslash for argv[0] because the path was created by CRT.
chdir is the same as _chdir; deprecation warnings have low level (ignored now, and could be disabled).
strdup (or _strdup) and truncation with null character could give shorter code.

But there is a catch.
Full path for a data file may be specified in command line; and the data may contain paths relative to the file's location.
Which means, chdir should be to the data file's path, not executable's path. Also (forward) slashes might be here.
Unless this line

You need to run the tests suites from the tests directory.

means "executable must be copied to tests directory" - which is not always a great idea.
Another trouble:
const char *default_filename = "./test_suite_ssl.datax";
With the current code this path in command line means not what might be expected:
tests/test_suite_ssl ./test_suite_ssl.datax

https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/864dee05300797842f7ab6b06c43e747cb759e78/tests/suites/host_test.function#L446-L449 This code correctly changed directory for me; verified with `_getcwd()`. Certainly backslash for `argv[0]` because the path was created by CRT. `chdir` is the same as `_chdir`; deprecation warnings have low level (ignored now, and could be disabled). `strdup` (or `_strdup`) and truncation with null character could give shorter code. But there is a catch. Full path for a data file may be specified in command line; and the data may contain paths relative to the file's location. Which means, `chdir` should be to the data file's path, not executable's path. Also (forward) slashes might be here. Unless this line > You need to run the tests suites from the `tests` directory. means "executable must be copied to `tests` directory" - which is not always a great idea. Another trouble: `const char *default_filename = "./test_suite_ssl.datax";` With the current code this path in command line means not what might be expected: `tests/test_suite_ssl ./test_suite_ssl.datax`
irwir commented 2025-06-04 08:10:48 -05:00 (Migrated from github.com)

To verify that only tests were failing, one final check was attempted with the line #include <winsock2.h> commented out. Build process would not start automatically, hence the commit was reverted.

But that code isn't implemented for Windows (patches welcome).

It is hard to provide useful patches without decent understanding of the testing structure and the future plans.

To verify that only tests were failing, one final check was attempted with the line `#include <winsock2.h>` commented out. Build process would not start automatically, hence the commit was reverted. > But that code isn't implemented for Windows (patches welcome). It is hard to provide useful patches without decent understanding of the testing structure and the future plans.
irwir commented 2025-07-26 06:56:48 -05:00 (Migrated from github.com)

Currently this PR seems to be blocked by a flaw in test code.
If so, are there any plans to move on?

Currently this PR seems to be blocked by a flaw in test code. If so, are there any plans to move on?
gilles-peskine-arm commented 2025-07-27 09:42:30 -05:00 (Migrated from github.com)

This pull request is blocked because it fails the CI. It's not a flaw in test code: the patch makes the existing code fail to compile. Since it only happens on Windows, which most of us are unfamiliar with, this is unlikely to progress unless a Windows expert can fix the problem.

This pull request is blocked because it fails the CI. It's not a flaw in test code: the patch makes the existing code fail to compile. Since it only happens on Windows, which most of us are unfamiliar with, this is unlikely to progress unless a Windows expert can fix the problem.
irwir commented 2025-07-28 06:15:44 -05:00 (Migrated from github.com)

This pull request is blocked because it fails the CI. It's not a flaw in test code: the patch makes the existing code fail to compile.

Compilation had been fixed months ago (the second commit). Since then the test_suite_ssl failed in every test that required to read a certificate file. With a bit of code tweaking test_suite_ssl passed all 854 tests (VS 2017).
The history is in the above conversation.
Outdated build logs had been deleted, but for verification you could run the process again.

> This pull request is blocked because it fails the CI. It's not a flaw in test code: the patch makes the existing code fail to compile. Compilation had been fixed months ago (the second commit). Since then the `test_suite_ssl` failed in every test that required to read a certificate file. With a bit of code tweaking `test_suite_ssl` passed all 854 tests (VS 2017). The history is in the above conversation. Outdated build logs had been deleted, but for verification you could run the process again.
gilles-peskine-arm commented 2025-07-28 06:47:19 -05:00 (Migrated from github.com)

Ok, I've kicked a new CI run.

Ok, I've kicked a new CI run.
irwir commented 2025-07-28 09:41:44 -05:00 (Migrated from github.com)

New kind of errors in build (exception in compiler?).
Synced PR with Mbed-TLS/TF-PSA-Crypto:development branch.

New kind of errors in build (exception in compiler?). Synced PR with `Mbed-TLS/TF-PSA-Crypto:development` branch.
irwir commented 2025-07-29 16:24:13 -05:00 (Migrated from github.com)

Rebased the PR and somehow all tests had passed.

Rebased the PR and somehow all tests had passed.
irwir commented 2025-07-30 04:29:11 -05:00 (Migrated from github.com)

It would be nice to add under the Windows condition this definition:

#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endif

But the macro must be added into name checker's exclusion list.

It would be nice to add under the Windows condition this definition: > #ifndef WIN32_LEAN_AND_MEAN > #define WIN32_LEAN_AND_MEAN > #endif > But the macro must be added into name checker's exclusion list.
gilles-peskine-arm commented 2025-07-30 04:32:23 -05:00 (Migrated from github.com)

But the macro must be added into name checker's exclusion list.

That needs a patch to https://github.com/Mbed-TLS/mbedtls-framework/blob/main/scripts/check_names.py

> But the macro must be added into name checker's exclusion list. That needs a patch to https://github.com/Mbed-TLS/mbedtls-framework/blob/main/scripts/check_names.py
irwir commented 2025-07-30 05:30:41 -05:00 (Migrated from github.com)

At line 343 like this?

exclusions = (
"asm", "inline", "EMIT", "CRT_SECURE_NO_DEPRECATE", "MULADDC",
"WIN32_LEAN_AND_MEAN"
)

At line 343 like this? > exclusions = ( > "asm", "inline", "EMIT", "_CRT_SECURE_NO_DEPRECATE", "MULADDC_", > "WIN32_LEAN_AND_MEAN" > ) >
gilles-peskine-arm commented 2025-07-30 05:33:19 -05:00 (Migrated from github.com)

Yes, that looks right, WIN32_LEAN_AND_MEAN and _CRT_SECURE_NO_DEPRECATE have very similar roles.

Yes, that looks right, `WIN32_LEAN_AND_MEAN` and `_CRT_SECURE_NO_DEPRECATE` have very similar roles.
irwir commented 2025-08-05 07:49:18 -05:00 (Migrated from github.com)

If this was merged, <windows.h> and <winsock2.h> could be removed from .c files in TF-PSA-Crypto and mbedtls.

If this was merged, `<windows.h>` and `<winsock2.h>` could be removed from `.c` files in `TF-PSA-Crypto` and `mbedtls`.
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'irwir/winsock2:irwir/winsock2'
git switch 'irwir/winsock2'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'irwir/winsock2'
git switch 'irwir/winsock2'
git rebase 'development'
git switch 'development'
git merge --ff-only 'irwir/winsock2'
git switch 'irwir/winsock2'
git rebase 'development'
git switch 'development'
git merge --no-ff 'irwir/winsock2'
git switch 'development'
git merge --squash 'irwir/winsock2'
git switch 'development'
git merge --ff-only 'irwir/winsock2'
git switch 'development'
git merge 'irwir/winsock2'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
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

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!262
No description provided.