gmtime_r and mbedtls_time on Windows #1198

NWilson · 2017-12-05T12:13:34Z

This PR fixes a couple of issues:

mbedTLS should not be calling gmtime_r. Even with a mutex, the call is simply unsafe (other threads in the application could be calling it, obviously without holding mbedTLS's mutex). It's simply not appropriate to be using non-thread-safe APIs in a security library.
On Windows, the mbedtls_time platform abstraction does not work, because for some reason the time() API is not used on Windows? I can't think of any reason not to use it, the Windows time implementation ought to go through the same platform abstraction layer as any other feature.
The second commit fixes the build with -std=c99. There are a couple of existing places in the code where this is broken, plus gmtime_r adds another one.

Issue: #704

RonEld · 2017-12-05T12:56:03Z

@NWilson Thank you for your continuous contributions!
We will have a look at it.

k-stachowiak

The PR introduces valuable changes, especially the synchronization problem fix. Additionally it cleans up some legacy code, however before it is merged two things must be done:

please remove the obsolete mutex definition,
add entry in the changelog crediting yourself as the contributor.

Also for the sake of backporting the removal of the WinAPI timing function use should be considered before this can be merged.

k-stachowiak · 2018-03-28T07:49:48Z

include/mbedtls/threading.h

@@ -96,7 +96,6 @@ extern int (*mbedtls_mutex_unlock)( mbedtls_threading_mutex_t *mutex );
 * Global mutexes
 */
 extern mbedtls_threading_mutex_t mbedtls_threading_readdir_mutex;
-extern mbedtls_threading_mutex_t mbedtls_threading_gmtime_mutex;


This change needs to be accompanied by an analogous change in threading.c. The remaining definition is the reason for the CI failure.

k-stachowiak · 2018-03-28T08:19:16Z

library/x509.c

 #if defined(MBEDTLS_HAVE_TIME)
 #include "mbedtls/platform_time.h"
 #endif
-
-#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)


This change makes the code a lot cleaner, however it will prevent us from backporting it as it definitely breaks WinCE compatibility. If you whish this PR to be backported to 2.7 and 2.1 versions please withdraw this change. Otherwise it is good.

I'm happy with no backporting, so I've left this change in. It's not an urgent fix.

I think this is a functional change to a feature, and is more an improvement than a bug fix, so no backport is necessary in my view.

k-stachowiak · 2018-03-28T13:23:22Z

@NWilson Thanks for your contribution! I checked for the reason the CI fails and I found a minor issue with the PR. I've put more detailed commets in the form of a review. Would you be willing to introduce the suggested changes?

NWilson · 2018-03-29T11:38:04Z

I've updated the PR by rebasing it, removing the mutex definition, and re-running the tests. Thanks for the review.

Sorry for lumping two different issues together here (gmtime_r and -std=c99 build), I only did them together because gmtime_r has various macro visibility requirements, and I wanted to test with those, and found that the rest of mbedTLS doesn't respect the POSIX visibility requirements!

k-stachowiak · 2018-03-29T11:46:35Z

Thank you for a quick response (a lot quicker than ours, sorry for the delay on our side)! The PR looks good now; I will arrange for the ultimate reviews once we've got a result from the CI.

AndrzejKurek

I would opt for adding a test using -std=c99.
If we're extending the compatibility, it would be nice to make sure it does not break in the future. What do you think, @sbutcher-arm ?

NWilson · 2018-04-03T20:23:21Z

I'm not sure how to add a test - it would involve adding something to the scripts run from .travis.yml I guess, since it's not a code feature per-so, more a test of whether the project builds under various different CFLAGS. I can't find anything quite like that at the moment.

NWilson · 2018-05-23T11:35:12Z

Bump - there are no existing tests for compilation using -std=c99, is it necessary to add such tests for the PR to be accepted?

simonbutcher

Just a couple of minor review point.

simonbutcher · 2018-04-03T17:40:24Z

library/entropy_poll.c

@@ -19,6 +19,11 @@
 *  This file is part of mbed TLS (https://tls.mbed.org)
 */

+#if defined(__linux)


This should be __linux__. I believe __linux is obsolete, although I haven't found an authoritative source on this.

Well spotted, I hadn't realised, but you're right.

simonbutcher · 2018-04-03T17:41:40Z

library/x509.c

 #if defined(MBEDTLS_HAVE_TIME)
 #include "mbedtls/platform_time.h"
 #endif
-
-#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)


I think this is a functional change to a feature, and is more an improvement than a bug fix, so no backport is necessary in my view.

simonbutcher · 2018-05-30T11:48:28Z

library/net_sockets.c

@@ -19,6 +19,11 @@
 *  This file is part of mbed TLS (https://tls.mbed.org)
 */

+/* Enable definition of getaddrinfo() even when compiling with -std=c99. Must
+ * be set before config.h, which pulls in features.h indirectly.  Harmless on


I'm not sure what you mean by features.h. Do you mean MBEDTLS_USER_CONFIG_FILE? Or do you mean a glibc specific header? Could you please clarify the comment?

It's the Linux features.h header, which is part of the Linux POSIX visibility system. See man feature_test_macros. I'll clarify the comment.

simonbutcher · 2018-05-30T12:16:14Z

I'm a bit concerned how this PR will work with Mbed OS, and what support there is in Mbed OS for gmtime_r(). There is a related PR over in Mbed OS to enable RTC support, and I'd like this not to break that.

cc: @k-stachowiak

Also in response to @NWilson's question concerning the --std=c99 test, if it passes the CI and all.sh it's fine.

k-stachowiak · 2018-05-30T15:35:00Z

@sbutcher-arm @NWilson I can test the two PRs interaction on an RTC enabled device on Monday.

NWilson · 2018-05-30T15:40:11Z

I can't find the relevant mbedOS documentation. What libc does it use? It looks like it might use newlib in future, but doesn't currently, and I can't find any reference to the standard C functions provided by mbedOS.

Unless mbedOS is living under a rock, gmtime_r should be available if gmtime is.

This shouldn't have any relation to Real-Time Clock support; gmtime is a hardware-independent algorithm that doesn't need to know about the current time at all.

I agree we don't want to break mbedOS, but I wouldn't expect it to (I can't test given I don't have any ARM boards or simulators set up).

k-stachowiak · 2018-06-04T12:36:03Z

I have verified in practise that the gmtime_r function produces results consistent with gmtime and therefore I sustain my approval for the PR. @sbutcher do you think the PR can be approved?

simonbutcher · 2018-06-12T13:45:32Z

Following @NWilson additional commits, can you please re-review?

Dismissing review, as it's stale due to subsequent work.

k-stachowiak

I found one blocker (missing define in a Makefile - easily fixable), one potential minor issue (unnecessary constant in one of the defines), and have a minor suggestion.

I must request changes due to the blocking issue.

k-stachowiak · 2018-06-13T13:25:12Z

library/x509.c

@@ -29,6 +29,10 @@
 *  http://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
 */

+/* Ensure gmtime_r is available even with -std=c99; must be included before
+ * config.h, which pulls in glibc's features.h. Harmless on other platforms. */
+#define _XOPEN_SOURCE 500


I can't find a reference saying that _XOPEN_SOURCE needs to be defined as 500 for gm_time_r; it merely needs to be defined. Unless there is an actual need for the specific value please remove the 500.

@k-stachowiak You are only partially right. Removing the 500 literal will cause other standard functions not to be available during the build.

k-stachowiak · 2018-06-13T14:03:02Z

tests/CMakeLists.txt

+# Enable definition of various functions used throughout the testsuite
+# (hostname, strdup, fileno...) even when compiling with -std=c99. Harmless
+# on non-POSIX platforms.
+add_definitions("-D_POSIX_C_SOURCE=200809L")


This must also be added in the Makefile.

This could be provided in tests/scripts/generate_code.pl for the consistency - having all the feature macros in the source files. However I do not have a strong opinion on this.

simonbutcher · 2018-06-21T10:51:31Z

@NWilson - As ever, your contributions are very welcome. If you don't have time for responding to the review comments or doing any rework, please let us know and we'll pick this up and finish it off for you.

In each place where POSIX/GNU functions are used, the file must declare that it wants POSIX functionality before including any system headers.

NWilson · 2018-06-25T11:12:00Z

Sorry, it just took me a while to get round to it! The changes were small, I've done them now and re-run the build. Also rebased to fix a conflict in the ChangeLog file.

I didn't edit the commits, the changes are contained in the final commit for ease of reviewing. Thanks!

simonbutcher · 2018-06-26T09:06:22Z

Thanks @NWilson! Hopefully we can get it in this time.

k-stachowiak

Thank you for addressing the comments. Unfortunately I was wrong on one of the remarks, in the way that it breaks our build in gcc 4.8.4. Could you please revert the change that I pointed out in a review comment? Otherwise the PR looks good to me.

k-stachowiak · 2018-06-26T11:46:42Z

library/x509.c

@@ -31,7 +31,7 @@

 /* Ensure gmtime_r is available even with -std=c99; must be included before
 * config.h, which pulls in glibc's features.h. Harmless on other platforms. */
-#define _XOPEN_SOURCE 500
+#define _XOPEN_SOURCE


Apparently I was overzealous with request for this change. It turns out that snprintf requires one of the following:

_BSD_SOURCE || _XOPEN_SOURCE >= 500 || _ISOC99_SOURCE || _POSIX_C_SOURCE >= 200112L

So unfortunately this change needs to be reverted.

OK, I've rolled back that change.

k-stachowiak

LGTM now.

simonbutcher · 2018-06-29T11:32:51Z

retest

k-stachowiak · 2018-07-06T12:54:56Z

I have investigated the FreeBSD build issue on a VM. The problem is, that, contrary to what has been documented in some places, _XOPEN_SOURCE >= 500 is not always enough to enable snprintf( ).

Looking here reveals, that in order to unlock this part of the API, we need to get the __ISO_C_VISIBLE symbol to resolve to at least 1999, which based on this leads to a conclusion that setting _POSIX_C_SOURCE to at least 200112 should do.

@NWilson For your convenience, I have submitted a PR with a relevant change to your clone. If you don't have any objections, you may merge it there, which in turn should lead to the commit being submitted here.

simonbutcher · 2018-07-09T12:07:40Z

retest

k-stachowiak · 2018-07-11T07:38:09Z

@sbutcher-arm @AndrzejKurek The CI tests have passed. Could you please review the new changes that made it possible?

…cleanup [Backport] Buffer sharing cleanup

RonEld added bug CLA valid enhancement labels Dec 5, 2017

k-stachowiak self-assigned this Mar 27, 2018

k-stachowiak self-requested a review March 27, 2018 13:19

k-stachowiak suggested changes Mar 28, 2018

View reviewed changes

NWilson force-pushed the gmtime_r branch from d358d50 to c420a69 Compare March 29, 2018 11:34

NWilson force-pushed the gmtime_r branch from c420a69 to 430335c Compare March 29, 2018 11:42

k-stachowiak previously approved these changes Mar 30, 2018

View reviewed changes

k-stachowiak requested a review from AndrzejKurek March 30, 2018 09:20

AndrzejKurek reviewed Mar 30, 2018

View reviewed changes

simonbutcher suggested changes May 30, 2018

View reviewed changes

NWilson force-pushed the gmtime_r branch from 430335c to 1436228 Compare May 30, 2018 15:58

simonbutcher added the needs-review Every commit must be reviewed by at least two team members, label Jun 12, 2018

k-stachowiak suggested changes Jun 13, 2018

View reviewed changes

simonbutcher added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 13, 2018

Fix build using -std=c99

2682edf

In each place where POSIX/GNU functions are used, the file must declare that it wants POSIX functionality before including any system headers.

NWilson force-pushed the gmtime_r branch from 1436228 to f592774 Compare June 25, 2018 11:10

simonbutcher added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 26, 2018

k-stachowiak suggested changes Jun 26, 2018

View reviewed changes

Address review comments - tidy usage of macros to use minimal values

61fa436

NWilson force-pushed the gmtime_r branch from f592774 to 61fa436 Compare June 28, 2018 16:10

k-stachowiak previously approved these changes Jun 29, 2018

View reviewed changes

Enable snprintf on FreeBSD

a5fbfd7

NWilson dismissed k-stachowiak’s stale review via a5fbfd7 July 8, 2018 12:22

AndrzejKurek approved these changes Jul 11, 2018

View reviewed changes

simonbutcher approved these changes Jul 11, 2018

View reviewed changes

simonbutcher added approved Design and code approved - may be waiting for CI or backports approved for design and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 11, 2018

simonbutcher merged commit a5fbfd7 into Mbed-TLS:development Jul 24, 2018

gilles-peskine-arm mentioned this pull request Aug 1, 2018

Build fails if gmtime_s() or gmtime_r() are not available #1907

Closed

andresag01 mentioned this pull request Aug 7, 2018

Use gmtime() when neither gmtime_r nor gmtime_s are available #1927

Merged

simonbutcher mentioned this pull request Sep 21, 2018

Use gmtime_r #704

Closed

NWilson deleted the gmtime_r branch September 22, 2018 16:15

gilles-peskine-arm mentioned this pull request Apr 30, 2020

Backport 2.7: Clarify that the full config enables everything that can be tested together #3295

Merged

This was referenced Sep 8, 2020

_POSIX_C_SOURCE fixes #3645

Closed

Allow builds with -std=c99 on LTS branch 2.7 #3656

Closed

minosgalanakis pushed a commit that referenced this pull request Mar 28, 2024

Merge pull request #1198 from tom-daubney-arm/backpot_buffer_sharing_…

b596eb6

…cleanup [Backport] Buffer sharing cleanup

gmtime_r and mbedtls_time on Windows #1198

gmtime_r and mbedtls_time on Windows #1198

Conversation

NWilson commented Dec 5, 2017

RonEld commented Dec 5, 2017

k-stachowiak left a 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

k-stachowiak commented Mar 28, 2018

NWilson commented Mar 29, 2018

k-stachowiak commented Mar 29, 2018

AndrzejKurek left a comment

Choose a reason for hiding this comment

NWilson commented Apr 3, 2018

NWilson commented May 23, 2018

simonbutcher left a 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

Choose a reason for hiding this comment

simonbutcher commented May 30, 2018

k-stachowiak commented May 30, 2018

NWilson commented May 30, 2018

k-stachowiak commented Jun 4, 2018

simonbutcher commented Jun 12, 2018

k-stachowiak left a 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

simonbutcher commented Jun 21, 2018

NWilson commented Jun 25, 2018

simonbutcher commented Jun 26, 2018

k-stachowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-stachowiak left a comment

Choose a reason for hiding this comment

simonbutcher commented Jun 29, 2018

k-stachowiak commented Jul 6, 2018

simonbutcher commented Jul 9, 2018

k-stachowiak commented Jul 11, 2018