Hi and thank you for your report. PR858 proposed a way for handling windows sockets but unfortunately it was never merged.

Could you you please provide some more context on how it affects your system? is it compilation warnings you are getting while intergrating?

This is a low priority issue and it is unlikely we will be able to do it soon. I am not however closing it, since we will happily accept community contributions on it (cherry picking the relevant commmit from the historic PR)

PR858

Networking part was a step in the right direction, but invalid socket check was not fully correct.

Could you you please provide some more context on how it affects your system? is it compilation warnings you are getting while intergrating?

It was described in the opening message, was it not?
When starting at about version 2.23, code would not compile for 64-bit Windows.
There was an old PR, which stays open as "historical-reviewed".
Still keep using modified networking code, because storing 64 bits in 32 is a bad practice.

So I do not see how this issue could be tagged "help-wanted" and "good-first-issue" while a design decision was required in the first place.
At "low priority", this most probably would miss v4.0. Such PR might imply (minor) API break for networking, then the nearest possibility would be v5.0 or later?

@irwir Would you be willing to propose a patch, at least for development to be first release in 4.0? Is there any question on the design at the moment, since there is no constraint on API or ABI compatibility?

Going by your description (I'm not familiar with Windows APIs), it seems there is a definite bug on Windows, which could possibly remain latent until a new Windows version or a new system configuration causes it to use a socket handle that's outside the range [0, 2^31-1]. Do you have any suggestion on how to fix it? If there's no other reasonable way, a Windows-only (64-bit-Windows-only?) ABI change might be acceptable, we'd have to discuss this further. Would the in-memory layout of mbedtls_net_context have to change? What are the rules for struct padding on Windows (maybe different between CPU architectures? for different compilers?)?

Regarding #858, it had a very ambitious objective, which in itself is not very important (just warnings, lots of warnings). In fact I would consider /W4 compliance to be unrealistic, based on my experience with MSVC. (Nowadays does that even allow memcpy and friends?) OTOH this here is a genuine bug, so we don't consider it low priority.

Re: PR858. The adjective ambitious applies naturally here. The fix was still partial, some warnings had to be disabled. Other warnings, like related to type casts, were fine to accept for cleaner code.

it seems there is a definite bug on Windows

Certain values were promised by MS to be 32 bits even in 64-bit platforms, but never saw such claims about socket handles, let alone to keep the value within 31 bits. Then it is a bug, even if never seen in practice.

Do you have any suggestion on how to fix it?

A suggestion exists in the mentioned PR #1427.
There was no reply to my query about its perspectives.
Also remained unanswered an offer to split the PR into two parts: networking and UTF-16LE for path names.

The changes for sockets mostly touch net_socket.* files.
Library build and tests required Winsock2 definitions to be included globally, and this was made in config.h as there was no better alternative (can be in build_info.h now?).

Please take a look to recall the general idea.
Should this way be found acceptable, upgrade to the current code base would be ovbivous.

Then it is a bug, even if never seen in practice.

If it's never seen in practice, then I think we won't fix it in 3.6 (which only has about two years left of support). But 4.0 would be a good time to future-proof against future evolution of Windows.

A suggestion exists in the mentioned PR #1427.
There was no reply to my query about its perspectives.

Sorry. Unfortunately the answer is always “we don't have time for this” 😞 . But a pull request that changes 3 files would have a much bigger chance than one that changes 22 files.

Library build and tests required Winsock2 definitions to be included globally, and this was made in config.h as there was no better alternative (can be in build_info.h now?).

I'm not sure why anything would be needed outside of net.h, but yes, since 3.0 you can rely on build_info.h being included before any other Mbed TLS header. (But not before any other header that the application chooses to include, of course.)

So would you mind making a pull request with just the socket part? I think we can find time for that. The Unicode names on the other hand would be a very hard sell.

I'm not sure why anything would be needed outside of net.h, but yes, since 3.0 you can rely on build_info.h being included before any other Mbed TLS header.

Winsock2 must be included before other Windows definitions to avoid defaulting to Winsock1.
This partially explains why the current macro implementation of zeroize is a failure on Windows.

The Unicode names on the other hand would be a very hard sell.

Development is heavily biased towards *nix, while Windows still has a noticeable share of library users.
Conversions between ANSI and Unicode becomes application developer's responsibility and might have known issues.
Undeniably, certain changes should be applied to make example (and test?) applications code fully portable - but this is only about path names.

This partially explains why the current macro implementation of zeroize is a failure on Windows.

That sounds bad. I guess you're referring to #8490 ? It's not clear to me in this thread what is wrong. Would you be willing to submit a patch for that? With explanations for reviewers who aren't at all familiar with Windows, please.

Development is heavily biased towards *nix

Not really. Most of us know Unix better than Windows, but the platforms that are prioritized (both in terms of library architecture and in terms of developer time) are embedded platforms such as microcontrollers and trusted enclaves. We develop and test primarily on Linux because it's convenient, but it isn't really our main target platform.

Conversions between ANSI and Unicode becomes application developer's responsibility and might have known issues.
Undeniably, certain changes should be applied to make example (and test?) applications code fully portable - but this is only about path names.

Oh, ideally, yes, absolutely. But I want to be realistic. Fixing file name encoding when reading a certificate database just isn't going to have enough priority that it gets reviewed. A relatively simple patch that fixes a potential major bug (socket) or a security weakness (zeroize) has a reasonable chance.

I guess you're referring to #8490 ? It's not clear to me in this thread what is wrong.

Question, 'What was right?' might be asked.
Yet again, there were two PRs (1644, 1760) - design choice question remained unanswered. Both PRs were doing as planned, and both had been closed rather formally.
The major point was to make zeroize macro the default as the most efficient.
However, in the current code such macro requires additional user's efforts, and that trick had been merged without a single try on Windows.

With the attempted macro, Visual Studio could not compile the library, then would not link.
An unusable library adds no security risks, but this is not a fortunate situation.
The code would not magically fix itself, and I cannot imagine a minor 'patch' for repairs.
Something along the lines of the old PRs could be feasible.

We develop and test primarily on Linux because it's convenient

This is fine, however, combination of better knowledge of Unix, usage of typical Unix toolchain, common references to limited Windows experience - mostly confirm my point.

Fixing file name encoding when reading a certificate database just isn't going to have enough priority that it gets reviewed.

The larger gets the library, the more efforts were required to straighten things out (all mentioned PRs were made in 2018).

When calling the library, on Windows user applications have to convert paths from native UTF-16LE to ANSI.
A security may be at risk when different Unicode characters correspond to the same ANSI character.

Back to sockets; design question.
To prevent redefinitions, winsock2.h ought to be included as early as possible. The innermost build_info.h is in tf-psa-crypto\include\tf-psa-crypto\, and the directive is to be placed before reading PSA crypto configuration.
This means, tf-psa-crypto would be changed.
Is this acceptable?
Early include also makes possible to remove some Windows headers from other files, but this should be in other PRs.

Early include also makes possible to remove some Windows headers from other files, but this should be in other PRs.

Test build of the VS solution passed successfully with all windows.h commented out from the library files, and WIN32_LEAN_AND_MEAN and VC_EXTRALEAN were defined to speed up compilation.
No full testing, but at least server2 and client2 examples connected.