Skip to content

implement gettime_secs for Windows (_MSC_VER) in tests/api.c#6733

Merged
JacobBarthelmeh merged 6 commits intowolfSSL:masterfrom
gojimmypi:windows-gettime_secs
Sep 11, 2023
Merged

implement gettime_secs for Windows (_MSC_VER) in tests/api.c#6733
JacobBarthelmeh merged 6 commits intowolfSSL:masterfrom
gojimmypi:windows-gettime_secs

Conversation

@gojimmypi
Copy link
Copy Markdown
Contributor

@gojimmypi gojimmypi commented Aug 25, 2023

Description

When using cmake on Windows, the call to gettime_secs fails as this is not implemented on the Windows platform.

This change checks for the Windows environment via the _MSC_VER and conditionally defines an alternate calculation.

As with #6600, but on Windows: Open a Visual Studio Developer Command Prompt:

cd  \workspace\wolfssl
mkdir out
cd out
cmake ..
cmake --build .

There are many warnings when building. This is a separate issue, but for reference messages such as :

C:\workspace\wolfssl\src\tls.c(2162): message : index '' range checked by comparison on this line [C:\test\wolfssl-no-openssl\out\wolfssl.vcxproj]
C:\workspace\wolfssl\src\tls.c(2229): message : feeds memory load on this line [C:\test\wolfssl-no-openssl\out\wolfssl.vcxproj]
C:\workspace\wolfssl\src\tls.c(4208): warning C4706: assignment within conditional expression [C:\test\wolfssl-no-openssl\out\wolfssl.vcxproj]
C:\workspace\wolfssl\src\tls.c(2180): warning C5045: Compiler will insert Spectre mitigation for memory load if /Qspectre switch specified [C:\test\wolfssl-no-openssl\out\wolfssl.vcxproj]

and

C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\shared\ws2def.h(269,1): warning C4820: '_SOCKET_ADDRESS': '4' bytes padding added after data member 'iSockaddrLength' [C:\workspace\wolfssl\out\client.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\shared\ws2def.h(278,31): warning C4820: '_SOCKET_ADDRESS_LIST': '4' bytes padding added after data member 'iAddressCount' [C:\workspace\wolfssl\out\client.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\shared\ws2def.h(691,42): warning C4820: '_WSABUF': '4' bytes padding added after data member 'len' [C:\workspace\wolfssl\out\client.vcxproj]

So to see any errors beyond the build warnings, just run cmake --build . a second time, or suppress the warnings.

Fixes zd# n/a

Testing

Add the source code function to something like client example on Windows, build with 'cmake' and observe output:

printf("\n%0f\n", TimeNowInMilliseconds());

and compare to Linux command:

date +%s

Run examples via:

.\examples\client\Debug\client.exe

You may need to manually copy the wolfSSL DLL files from \Debug to \examples\client\Debug.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@gojimmypi gojimmypi self-assigned this Aug 25, 2023
@gojimmypi gojimmypi force-pushed the windows-gettime_secs branch from 7645f11 to d19548c Compare August 26, 2023 00:12
Comment thread tests/api.c
LIBCALL_CHECK_RET(gettimeofday(&tv, 0));
#if defined(_MSC_VER) && defined(_WIN32)
{
/* there's no gettimeofday for Windows, so we'll use system time */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be a call to XTIME() ? It's how wolfSSL is checking certificate validity on Windows and looks like could even replace the previous gettimeofday call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, good idea. But after going down that road I realized XTIME() is only granular to the level of whole seconds. In contrast, the double gettime_secs() is a double as it has 5 decimal places of fractional seconds.

I've updated this PR to instead pull out the TimeNowInMilliseconds() from tls13.c and make it a WOLFSSL_API in the internal.h file, thus allowing gettime_secs() in api.c to use TimeNowInMilliseconds().

We still lose 2 digits of precision, but I wonder how much more granular the measurements need to be than milliseconds? This change also results in gettime_secs() being available for many different platforms,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For greater precision it looks like there is the macro XTIME_MS(). The suggestion was more to avoid duplication of code if we had a way to collect the number of seconds/microseconds already. If a large refactor or new public API is needed to achieve that then please just use the initial approach of adding a new ifelse macro case in tests/api.c.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you help me understand how to use XTIME_MS()? It appears it is defined as m2mb_xtime_ms((tl)) and only in wc_port.h when gated with WOLFSSL_TELIT_M2MB and WOLFSSL_TLS13. The implementation of m2mb_xtime_ms appears to be very platform specific, using the m2mb_rtc_open("/dev/rtc0", 0).

I like the multi-platform implementation of TimeNowInMilliseconds() as having a common global millisecond timer for all platforms would be very handy, and not specific to only TLS1.3.

It's not so much that I wanted a new public API, but that was not only way I could see how to make it visible in api.c. I probably don't understand the wolfSSL visibility macros as well as I'd like.

Given this, how would you prefer that I proceed? Back to the initial approach adding Windows support? Do you think it is possible to get this working without being a WOLFSSL_API function?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please go back to the initial approach. Would prefer that over having the new public API. Sorry for the hassle the initial XTIME suggestion caused.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problem, it was a valid idea. It may be valuable to have a wolfSSL time library at some point.

For now in this PR I've reverted to the initial approach as requested.

@gojimmypi gojimmypi changed the title implement gettime_secs for Windows (_MSC_VER) in tests/api.c implement gettime_secs for other platforms in tests/api.c Aug 29, 2023
@gojimmypi gojimmypi changed the title implement gettime_secs for other platforms in tests/api.c implement gettime_secs for Windows (_MSC_VER) in tests/api.c Sep 9, 2023
@JacobBarthelmeh JacobBarthelmeh merged commit 63477bc into wolfSSL:master Sep 11, 2023
@gojimmypi gojimmypi deleted the windows-gettime_secs branch October 9, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants