Changes for Windows 64-bit builds #1427

irwir · 2018-03-08T20:11:05Z

Description

Sockets in the current code are assumed to be signed integers, while in Windows sockets are unsigned long or unsigned long long depending on configuration.
Macros and minor code changes were made for better compatibility with Visual Studio.
Also Unicode file names are better handled as wide character strings.

The goal was to try to change Windows-specific parts of the code, but leave common or non-Windows code intact.

Status

READY

Requires Backporting

NO

Which branch?

development

Additional comments

The code was tested in VS 2013/2015/2017, both 32- and 64-bit.
VS 2010 projects will need testing.

simonbutcher · 2018-03-09T17:12:54Z

Hi, and thanks for the contribution!

To accept the contribution and any future contribution we will need a Contributor’s Licence Agreement (CLA) signed by yourself or by whoever has the authority to do so if this is a contribution from a company.

For personal contributions, you can create an mbed account and accept an online agreement with a click through here, or you can find an agreement to sign and return to us, here, if the contribution is from a company or you prefer not to create an mbed account.

Thanks for your understanding.

Now - about the code. Looking through your PR, I can see you've made quite a few changes to the library to make it work better with Windows, and there are changes to change char to TCHAR as well as other API changes.

Mbed TLS is fundamentally a cross-platform library, and we want it to work as well on Windows as Linux as Android as a baremetal board. However, it is fundamentally a UTF-8 library, and at the moment we only support UTF-8. For accessing the filesystem I can see that's a problem in Windows.

The way we try to address platform dependencies is through the platform code, which can be replaced and substituted for code that suits the platform of your choice. So for those who can't use net_sockets.c, they can provide their own implementation of the same module.

Equally, the way to address issues of fopen() I think are to abstract those calls into the platform code. That's what @mazimkhan has been working on with his PR #1029, which abstracts away the filesystem calls.

I don't think #1029 which does similar things in a different way, will suit your needs because it still has char as the means to indicate the file path, when to suit Windows we probably need something close to tchar. Could you review that PR, and see if we can bring the changes and ideas you want into that PR?

irwir · 2018-03-09T18:31:27Z

Hi, and thanks for the reply

accept an online agreement with a click

You accepted the Contributor Agreement on Thu 08 Mar 2018.
It was done before making this PR. No more clicks available, please check if your registration system is working.

you've made quite a few changes

The idea was to change as little as possible, as in the third commit, which failed only because macro names did not conform to local standards.
I guess, socket code changes are very much contained within net_socket.* files.
If there are no objections from your side, it can be applied separately, this PR closed, and character size issue could be addressed later in other PR.
Please tell me your opinion.

it is fundamentally a UTF-8 library, and at the moment we only support UTF-8. For accessing the filesystem I can see that's a problem in Windows.

UTF-16LE is the native character format in Windows. UTF-8 would require encoding and decoding between char and wchar_t, at least in Unicode builds.
I will look what could be done here and of course willl take into account PR #1029.

PS. I revisited Windows-specific parts in x509_crt.c for example.
This is a strictly ANSI code, which unnecessarily converts ANSI strings to wide characters, but there is no slightest hint about UTF-8.
That should be changed, and it would be preferable to use Windows native formats in Windows-specific code.

Macros for function names might be better in lower case FIx in mbedtls_x509_crt_parse_file function

gilles-peskine-arm · 2018-03-14T18:41:16Z

include/mbedtls/config.h

+#define MBEDTLS_TCHAR TCHAR
+#define MBEDTLS_FOPEN _tfopen
+#else
+#define MBEDTLS__T(x) x


What is MBEDTLS__T supposed to mean? and MBEDTLS_TCHAR? Please use names that are meaningful on their own. Most Mbed TLS developers aren't familiar with the Windows APIs. Also, please document when to use those macros.

That is a tough job to comment on Windows-specific changes without knowing the platform.

What is MBEDTLS__T supposed to mean? and MBEDTLS_TCHAR?

Here is a short article covering the basics: https://msdn.microsoft.com/en-us/library/dybsewaf.aspx

Please use names that are meaningful on their own.

That might be redirected to Microsoft.
In the fist commit I tried to use original macro names defined in Windows, but had to comply to local standards and to add prefixes.

when to use those macros.

In short, do not use it in non-Windows code, because after macro expansion this code should be the same as it was before the requested changes were applied. That is why TCHAR becomes simply char and _T() is a no-op in non-Windows environment.

gilles-peskine-arm · 2018-03-14T18:42:15Z

include/mbedtls/config.h

+#else
+#define MBEDTLS__T(x) x
+#define MBEDTLS_TCHAR char
+#define MBEDTLS_FOPEN fopen


Note that #1029 also defines an abstraction for fopen. This and #1029 will need to be reconciled.

Yes, but code in this PR is already working, and #1029 is still in development, and Windows-specific part of it probably was never tested.

gilles-peskine-arm · 2018-03-14T18:44:42Z

library/x509_crt.c

-        return( MBEDTLS_ERR_X509_BAD_INPUT_DATA );
-
-    hFind = FindFirstFileW( szDir, &file_data );
+    hFind = FindFirstFile( szDir, &file_data );


#1029 also adds an abstraction for that. Please investigate how to reconcile this change. It might be best to base this PR from #1029, but beware that #1029 may still change before it gets merged.

Before trying to reconcile, it would be nice to get a compilable code in #1029.
I left a comment there, awaiting for changes.

#1029 also adds an abstraction for that.

The Windows-specific part of the mbedtls_x509_crt_parse_path function does not use any abstractions. It could be verified using cross-repository compare that this part of code has no differences with the current development branch.
Even more, there might be no gain in enforcing Unix-style abstraction here.

irwir · 2018-03-14T21:17:34Z

Sorry, I am getting curious.
As written above, CLA was already accepted before making this PR; and your site confirms that fact..
What is missing here that the request is still pending, and that soon would be a week?

simonbutcher · 2018-03-20T13:33:16Z

Hi @irwir

I can confirm that the Mbed clickthrough CLA has been accepted. Unfortunately, it's not integrated with Github, so the job of checkin a PR contributor against the mbed system is a manual step.

simonbutcher · 2018-03-26T11:10:25Z

@Patater - Please provide a design review.

Faless · 2018-04-04T12:52:58Z

library/x509_crt.c

@@ -72,6 +72,9 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <dirent.h>
+#define mbedtls_strlen     strlen
+#else
+#define mbedtls_strlen     _tcslen


This seems to cause undefined reference to '_tcslen' when building with MinGW-w64 (v5.0.2)

The issue should have been resolved: 973661f.
ssl_server.exe was built with portable istallation of MinGW, and connection tested in Firefox.

Faless · 2018-04-04T12:53:36Z

library/x509_crt.c

-    memcpy( filename, path, len );
-    filename[len++] = '\\';
+    memset( filename, 0, sizeof(filename) );
+    _tcsncpy( filename, path, len );


This seems to cause undefined reference to '_tcsncpy' when building with MinGW-w64 (v5.0.2)

Thanks for testing.
Could you suggest how to define these _tcslen and _tcslen in MinGW-w64?
In Visual Studio for Unicode builds it is
#define _tcslen wcslen
and
#define _tcsncpy wcsncpy

In ANSI builds
#define _tcslen strlen
and
#define _tcsncpy strncpy

Edit.
Found in MinGW repository, file string.h:
size_t __cdecl wcslen(const wchar_t *_Str);
wchar_t *wcsncpy(wchar_t * __restrict__ _Dest,const wchar_t * __restrict__ _Source,size_t _Count) __MINGW_ATTRIB_DEPRECATED_SEC_WARN;

Perhaps adding preprocessor definitions would be sufficient to build successfully in MinGW-w64?

irwir · 2023-04-18T09:57:37Z

Does this PR have any better future than staying in historically-reviewed category?

irwir added 6 commits March 8, 2018 21:11

Changes for Windows 64-bit builds

ff1fbc9

typdef should have been #define

d48b5fa

Trying a quick fix

26bd698

Changed macro names to conforming

5ea8ac8

Update macros in config-mini-tls.h too

177c83f

Fix placement of macros in net_socket.c

7dcedde

irwir added 2 commits March 10, 2018 00:37

Somewhat diminish the total amount of changes

5485977

Macros for function names might be better in lower case FIx in mbedtls_x509_crt_parse_file function

MBEDTLS_FOPEN had to be in uppercase.

d427af1

simonbutcher added enhancement CLA requested labels Mar 10, 2018

simonbutcher mentioned this pull request Mar 14, 2018

Update Windows Crypto APIs #1453

Open

2 tasks

gilles-peskine-arm reviewed Mar 14, 2018

View reviewed changes

simonbutcher added CLA valid and removed CLA requested labels Mar 20, 2018

mazimkhan self-assigned this Mar 21, 2018

Patater added the needs-design-approval label Mar 26, 2018

simonbutcher requested a review from Patater March 26, 2018 11:10

Patater self-assigned this Mar 28, 2018

irwir mentioned this pull request Apr 4, 2018

Only redefine _WIN32_WINNT macro when < 0x0501 #1555

Merged

2 tasks

Faless reviewed Apr 4, 2018

View reviewed changes

simonbutcher unassigned mazimkhan Sep 6, 2018

RonEld added the component-platform Portability layer and build scripts label Feb 18, 2019

tom-cosgrove-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label Feb 23, 2022

tom-cosgrove-arm assigned tom-cosgrove-arm and unassigned Patater Mar 4, 2022

daverodgman removed the historical-reviewing Currently reviewing (for legacy PR/issues) label Mar 11, 2022

daverodgman removed the CLA valid label May 13, 2022

daverodgman added the historical-reviewed Reviewed & agreed to keep legacy PR/issue label Sep 29, 2022

Trying to fix MinGW-w64 (v5.0.2) builds

84e8bec

irwir force-pushed the win64 branch from 973661f to 84e8bec Compare September 27, 2023 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for Windows 64-bit builds #1427

Changes for Windows 64-bit builds #1427

irwir commented Mar 8, 2018 •

edited

Loading

simonbutcher commented Mar 9, 2018 •

edited

Loading

irwir commented Mar 9, 2018 •

edited

Loading

gilles-peskine-arm Mar 14, 2018

irwir Mar 16, 2018 •

edited

Loading

gilles-peskine-arm Mar 14, 2018

irwir Mar 16, 2018

gilles-peskine-arm Mar 14, 2018

irwir Mar 15, 2018

irwir Mar 21, 2018

irwir commented Mar 14, 2018

simonbutcher commented Mar 20, 2018

simonbutcher commented Mar 26, 2018

Faless Apr 4, 2018 •

edited

Loading

irwir Apr 5, 2018

Faless Apr 4, 2018

irwir Apr 4, 2018 •

edited

Loading

irwir commented Apr 18, 2023

Changes for Windows 64-bit builds #1427

Are you sure you want to change the base?

Changes for Windows 64-bit builds #1427

Conversation

irwir commented Mar 8, 2018 • edited Loading

Description

Status

Requires Backporting

Which branch?

Additional comments

simonbutcher commented Mar 9, 2018 • edited Loading

irwir commented Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

irwir Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irwir commented Mar 14, 2018

simonbutcher commented Mar 20, 2018

simonbutcher commented Mar 26, 2018

Faless Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irwir Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

irwir commented Apr 18, 2023

irwir commented Mar 8, 2018 •

edited

Loading

simonbutcher commented Mar 9, 2018 •

edited

Loading

irwir commented Mar 9, 2018 •

edited

Loading

irwir Mar 16, 2018 •

edited

Loading

Faless Apr 4, 2018 •

edited

Loading

irwir Apr 4, 2018 •

edited

Loading