1406736 - Correct _POSIX_C_SOURCE-related break in MinGW

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Tom Ritter [:tjr]]

gmtime_r is only declared in MinGW if both _POSIX_C_SOURCE and _POSIX_THREAD_SAFE_FUNCTIONS are declared, while in pkixtestutil.cpp we only check for the first macro.

Comment 1

[Tom Ritter [:tjr]]

Attachment: Bug 1406736 Match MinGW's macro so we declare gmtime_r under MinGW too

Review commit: https://reviewboard.mozilla.org/r/187542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187542/

Comment 2

[Tom Ritter [:tjr]]

Comment on attachment 8916375 [details]
Bug 1406736 Match MinGW's macro so we declare gmtime_r under MinGW too

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187542/diff/1-2/

Comment 3

[Tom Ritter [:tjr]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abd9616406ea7ef7608358915f8c4c595bce9f68

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8916375 [details]
Bug 1406736 Match MinGW's macro so we declare gmtime_r under MinGW too

https://reviewboard.mozilla.org/r/187542/#review192772

Has the guard condition changed since bug 1119179?  That bug was about hiding the definition of `gmtime_r` under mingw, too...

Comment 5

[Tom Ritter [:tjr]]

Hm. No it hasn't. And Jacek last touched that in mingw.

> (Jacek Caban      2015-01-30 11:16:50 +0100 273) #if defined(_POSIX_C_SOURCE) && !defined(_POSIX_THREAD_SAFE_FUNCTIONS)
> (Jacek Caban      2015-01-30 11:16:50 +0100 274) #define _POSIX_THREAD_SAFE_FUNCTIONS 200112L
> (Jacek Caban      2015-01-30 11:16:50 +0100 275) #endif
> (Jacek Caban      2015-01-30 11:16:50 +0100 276)
> (Jacek Caban      2015-01-30 11:16:50 +0100 277) #ifdef _POSIX_THREAD_SAFE_FUNCTIONS
> (Jacek Caban      2015-01-05 14:00:18 +0100 278) __forceinline struct tm *__cdecl localtime_r(const time_t *_Time, struct tm *_Tm) {
> (Jacek Caban      2015-01-05 14:00:18 +0100 279)   return localtime_s(_Tm, _Time) ? NULL : _Tm;
> (Jacek Caban      2015-01-05 14:00:18 +0100 280) }
> (Jacek Caban      2015-01-05 14:00:18 +0100 281) __forceinline struct tm *__cdecl gmtime_r(const time_t *_Time, struct tm *_Tm) {
> (Jacek Caban      2015-01-05 14:00:18 +0100 282)   return gmtime_s(_Tm, _Time) ? NULL : _Tm;
> (Jacek Caban      2015-01-05 14:00:18 +0100 283) }
> (Jacek Caban      2015-01-06 13:24:49 +0100 284) __forceinline char *__cdecl ctime_r(const time_t *_Time, char *_Str) {
> (Yuta NAKAI       2015-02-10 21:10:02 +0900 285)   return ctime_s(_Str, 0x7fffffff, _Time) ? NULL : _Str;
> (Jacek Caban      2015-01-06 13:24:49 +0100 286) }
> (Jacek Caban      2015-01-07 12:31:42 +0100 287) __forceinline char *__cdecl asctime_r(const struct tm *_Tm, char * _Str) {
> (Jacek Caban      2015-01-06 13:24:49 +0100 288)   return asctime_s(_Str, 0x7fffffff, _Tm) ? NULL : _Str;
> (Jacek Caban      2015-01-06 13:24:49 +0100 289) }
> (Jacek Caban      2015-01-30 11:16:50 +0100 290) #endif


I'm a little confused about what's going on here. _POSIX_THREAD_SAFE_FUNCTIONS is #undef-ed and #defined in winpthread which is a change from 3 years ago; but it doesn't actually make sense.

If _POSIX_THREAD_SAFE_FUNCTIONS is not defined in MinGW, mingw won't define gmtime_r and we should define it. If it is defined in MinGW, _POSIX_THREAD_SAFE_FUNCTIONS won't be _unset_ by anything I can find, and we should not define it.

Jacek is there something I'm missing?

Comment 6

[Jacek Caban]

Sorry for late response. I did some Git history digging. I don't remember details of previous bugs. I can see that currently logic is partially broken on mingw-w64 side due to how pthread.h is integrated. _POSIX_THREAD_SAFE_FUNCTIONS is unconditionally defined in unistd.h (posix_unistd.h) in pthread-enabled toolchain. time.h checks for that and defines it when needed. It means that it will behave differently depending on include order. If you for example do:

#include <time.h>
#include <unitsd.h>

Then gmtime_r will not be declared in time.h, but _POSIX_THREAD_SAFE_FUNCTIONS will be defined. This should ideally be fixed in mingw-w64, but it's tricky due to how pthread integration works.


That said, proposed patch looks like a good workaround.

Comment 7

[Nathan Froyd [:froydnj]]

If Jacek is happy with how the patch looks, then I am happy with it too!

Just to make sure I understand this: if we get the include order of headers mixed up, mingw won't define gmtime_r, and we won't define a polyfill for gmtime_r, and the build will fail?  That seems...unfortunate, but I guess we don't have any better options than to just carefully ensure the headers are included in the correct order?

Comment 8

[Jacek Caban]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Just to make sure I understand this: if we get the include order of headers
> mixed up, mingw won't define gmtime_r, and we won't define a polyfill for
> gmtime_r, and the build will fail?

Yes, that's right.

> That seems...unfortunate, but I guess we
> don't have any better options than to just carefully ensure the headers are
> included in the correct order?
Ensuring correct includes order should fix it, but it's tricky with unified sources. Includes from other .cpp files may mess things (but we could just move problematic file out of unified sources). The patch that Tom proposed ensures that we will do the right thing in regards to declaring gmtime_r regardless of includes order (assuming mingw-w64 will stay compatible with current approach).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58d23d2b727e
Match MinGW's macro so we declare gmtime_r under MinGW too r=froydnj

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/58d23d2b727e