Updated 1.0.2 so thread safe gmtime_r/localtime_r are used on supported systems. #3609
Conversation
16850de to
40c8403
Compare
crypto/mem_dbg.c
Outdated
There was a problem hiding this comment.
If one takes this approach, one has to fix pointer handling on in 64-bit VMS build, see o_time.c in master.
crypto/o_time.c
Outdated
There was a problem hiding this comment.
Oh! Maybe 64-bit VMS is not supported in 1.0.2, in which case one doesn't have to go through pointer trouble mentioned above... @levitte, care to weigh in? BTW, !defined(OPENSSL_SYS_MACOSX) was contributed as "make OpenSSL compilable on MacOS/X". One can argue "oh! that was long time ago". Yes, but then it better be explicitly said, e.g. in commit message, preferably with reference when they implemented it or at least oldest version one verified. On the other hand one can argue that it's not the kind of things that one should do in branch that is in maintenance mode, i.e. make adjustment based on current status of nearby computer...
There was a problem hiding this comment.
Errr, 64-bit pointers is supported in 1.0.2, so yeah, this is something to consider.
There was a problem hiding this comment.
Revisiting this... I just realised that under "normal" circumstances, gmtime_r is undefined on VMS, so this section never gets built, which means that either the gmtime implementation will be used, or the much more VMSy implementation further down.
The condition above is incorrect, (!defined(OPENSSL_SYS_VMS) || defined(gmtime_r)) should really be (!defined(OPENSSL_SYS_VMS) || (__CRTL_VER >= 70000000 && (defined _POSIX_C_SOURCE || !defined _ANSI_C_SOURCE)))... funny thing is that exactly that condition is used to define VMS_GMTIME_OK
So I suggest that the VMS stuff needs a bit of renovation which is beyond the scope of this PR. Or leave it be, let it keep using gmtime (which is supposedly thread safe on VMS, due to internal thread local storage).
…mtime. Updated uses of gmtime to now call OPENSSL_gmtime instead. Used similar preprocessor logic to make sure localtime_r is called instead of localtime when applicable.
40c8403 to
8631b64
Compare
|
so what do we need to move this forward? VMS 64bit pointer? The original poster was only trying to fix Mac. @levitte any thoughts? |
|
Considering what I just commented, this should be fine for now. If we want to do things superbly correct, we might want to consider an internal |
|
Ping @openssl/committers. First review was done by @levitte - waiting for a second. |
…mtime. Updated uses of gmtime to now call OPENSSL_gmtime instead. Used similar preprocessor logic to make sure localtime_r is called instead of localtime when applicable. Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #3609)
|
Merged. 8552d91 |
macOS supports the thread safe
gmtime_rso I changed the logic inOPENSSL_gmtime()to make it choose that overgmtime.Calls to
gmtimehave been updated to useOPENSSL_gmtime()which makes the right decision on which function to use depending on the system.Updated calls to localtime to call localtime_r if supported on that system. (Used similar logic as
OPENSSL_gmtime()uses to determine which systems support it. Ifgmtime_ris supported so shouldlocaltime_r.)Checklist