Bug 5311 – localtime_r, gmtime_r... not available on Win32 (possibly other systems?)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Justin Haygood]

It's not available on Win32, but ya'll already knew that.

My suggestion:

in config.h, add a HAVE_XTIME_R (or something similar)

and in date_object.cpp someplace:

#if !HAVE_TIME_R
struct tm *gmtime_r(time_t *_clock, struct tm *_result)
{
  struct tm *p = gmtime(_clock);

  if (p)
    *(_result) = *p;

  return p;
}

struct tm *localtime_r(time_t *_clock, struct tm *_result)
{
  struct tm *p = localtime(_clock);

  if (p)
    *(_result) = *p;

  return p;
}
#endif // !HAVE_TIME_R

This could work, and is thread-safe I believe. I'm not gonna make a patch until
someone else makes suggestions.

CCing Kozol since he's the other Win32 guru (smarter than me definely)

[Krzysztof Kowalczyk]

I agree that *_r are missing on Win32 and this looks like a reasonable solution.

[Justin Haygood]

Created attachment 5285 [details]
Adds localtime_r, gmtime_r support

1. Adds localtime_r, gmtime_r support for Windows
2. Also adds define for snprintf (Windows is silly and calls its _snprintf)

[Eric Seidel (no email)]

Comment on attachment 5285 [details]
Adds localtime_r, gmtime_r support

Is localtime() threadsafe under windows??  Otherwise this would not be OK. 
Also, if so this shoudl be inside a __WIN32__ block, as this is certainly not
OK for unix systems, where localtime() is not threadsafe.

[Justin Haygood]

This code is in the #if WIN32 / #endif block that date_object.cpp aleady has.

I don't know if its threadsafe, but on Windows, threading is completely disabled
in JavaScriptCore since it was decided threading was not needed if its just used
by WebCore

Reference: http://www.opendarwin.org/pipermail/webkit-dev/2005-June/000063.html

(In reply to comment #3)
> (From update of attachment 5285 [details] [edit])
> Is localtime() threadsafe under windows??  Otherwise this would not be OK. 
> Also, if so this shoudl be inside a __WIN32__ block, as this is certainly not
> OK for unix systems, where localtime() is not threadsafe.
> 

[Geoffrey Garen]

Great solution!

You need to return _result, not p, though. p will be a pointer to gmtime's/localtime's static time_t buffer, 
which is not safe against re-entrancy. Luckily, _result already points to an independent copy of that data, 
so you can return it, instead.

Fix that and r=me.

[Justin Haygood]

Created attachment 5286 [details]
Adds localtime_r, gmtime_r support with reentrancy protection

[Geoffrey Garen]

Comment on attachment 5286 [details]
Adds localtime_r, gmtime_r support with reentrancy protection

r+, under the assumption that the comment above addresses Eric's concerns.

[Justin Haygood]

(In reply to comment #7)
> (From update of attachment 5286 [details] [edit])
> r+, under the assumption that the comment above addresses Eric's concerns.
> 

localtime/gmtime aren't really threadsafe, but JavaScriptCore is anything but
threadsafe when used on Windows at the moment, reason its disabled in config.h

[Justin Haygood]

Created attachment 5287 [details]
localtime_r, gmtime_r with locking

Same as above, but locks to make safer.

[Geoffrey Garen]

Comment on attachment 5287 [details]
localtime_r, gmtime_r with locking

r=me

[Eric Seidel (no email)]

Comment on attachment 5286 [details]
Adds localtime_r, gmtime_r support with reentrancy protection

removing +, after ggaren, jhaywood and I talked this out on irc.

[Justin Haygood]

Created attachment 5289 [details]
Finally...

namespacing sucks at times :-D

[Alexey Proskuryakov]

Comment on attachment 5289 [details]
Finally...

AFAIK, gmtime is thread-safe on Windows (uses thread local storage), so the
locks shouldn't be necessary.

[Justin Haygood]

Actually, _gmtime_s would be better than gmtime() for windows (and same with
localtime).

"Converts a time value to a structure. These are versions of _gmtime32,
_gmtime64 with security enhancements as described in Security Enhancements in
the CRT."

[Justin Haygood]

Created attachment 5290 [details]
localtime_r, gmtime_r implemented using secure gmtime/localtime

Adds security features, doesn't use locking since these are thread-safe

Also, it'll keep working after 2038 :-D.

[Justin Haygood]

Comment on attachment 5290 [details]
localtime_r, gmtime_r implemented using secure gmtime/localtime

>Index: kjs/date_object.cpp
>===================================================================
>RCS file: /cvs/root/JavaScriptCore/kjs/date_object.cpp,v
>retrieving revision 1.66
>diff -u -r1.66 date_object.cpp
>--- kjs/date_object.cpp	16 Dec 2005 23:01:33 -0000	1.66
>+++ kjs/date_object.cpp	26 Dec 2005 19:52:52 -0000
>@@ -59,6 +59,19 @@
> #define copysign(x, y) _copysign(x, y)
> #define isfinite(x) _finite(x)
> #define strncasecmp(x, y, z) strnicmp(x, y, z)
>+#define snprintf _snprintf
>+
>+struct tm *gmtime_r(time_t *_clock, struct tm *_result)
>+{
>+    _gmtime64_s(_result, _clock);
>+    return _result;
>+}
>+
>+struct tm *localtime_r(time_t *_clock, struct tm *_result)
>+{
>+    _localtime64_s(_result, _clock);
>+    return _result;
>+}
> #endif
> 
> namespace KJS {
>@@ -217,14 +230,21 @@
> 
> static UString formatTime(const tm &t)
> {
>+    int gmtoff;
>+#if !WIN32
>+    gmtoff = t.tm_gmtoff;
>+#else
>+    gmtoff = -(timezone / 60 - (t.tm_isdst > 0 ? 60 : 0 )) * 60;
>+#endif
>+
>     char buffer[100];
>-    if (t.tm_gmtoff == 0) {
>+    if (gmtoff == 0) {
>         snprintf(buffer, sizeof(buffer), "%02d:%02d:%02d GMT", t.tm_hour, t.tm_min, t.tm_sec);
>     } else {
>-        int offset = abs(t.tm_gmtoff);
>+        int offset = abs(gmtoff);
>         snprintf(buffer, sizeof(buffer), "%02d:%02d:%02d GMT%c%02d%02d",
>             t.tm_hour, t.tm_min, t.tm_sec,
>-            t.tm_gmtoff < 0 ? '-' : '+', offset / (60*60), (offset / 60) % 60);
>+            gmtoff < 0 ? '-' : '+', offset / (60*60), (offset / 60) % 60);
>     }
>     return UString(buffer);
> }

[Eric Seidel (no email)]

Comment on attachment 5290 [details]
localtime_r, gmtime_r implemented using secure gmtime/localtime

This is so outside my area of expertise it's not even funny.

the msdn documentation talks about how the _s versions of functiosn mostly just
add parameter checking, but that you need to call
_set_invalid_parameter_handler to setup handle the error cases.

Donno.

[Darin Adler]

Comment on attachment 5289 [details]
Finally...

I don't think the JSLock is relevant here. The calls gmtime and localtime could
be used on threads where no JavaScript is running at all, and the JSLock is
already held here inside the date code. So it doesn't do any good to have that
locking.

[Darin Adler]

Comment on attachment 5290 [details]
localtime_r, gmtime_r implemented using secure gmtime/localtime

Using the secure version is OK, although it doesn't help with the issue that
"_r" deals with, but why use the 64-bit version? The parameter is just a
time_t, so there's no benefit to using the 64-bit one.

The basic concept of the changes to work on Windows are good, but lets not land
this with the "64" variant.

[Darin Adler]

I think it's fine to use "plain old localtime" and "plain old gmtime" on systems that don't have the _r 
versions.

On Mac OS X, for example, either version is fine, because the non-r versions are threadsafe (by using a 
per-thread global instead of a true global).

[Darin Adler]

Comment on attachment 5287 [details]
localtime_r, gmtime_r with locking

The locking is wrong.

[Geoffrey Garen]

(In reply to comment #20)
> I think it's fine to use "plain old localtime" and "plain old gmtime" on systems that don't have the _r 
> versions.
> 
> On Mac OS X, for example, either version is fine, because the non-r versions are threadsafe (by using a 
> per-thread global instead of a true global).

Actually, we switched to the _r variants to fix a few bugs (on OS X). The issue isn't just thread safety. 
Rather, there are places in our code where localtime/gmtime gets called multiple times in the same stack. 
So if you don't use _r or something designed to behave like it, the first value on the stack gets trampled by 
the second call.

[Geoffrey Garen]

Another option around the bug I just mentioned is to replace calls like

struct tm *t = gmtime();

with calls like

struct tm t= *gmtime();

I think that would work on all platforms.

[Justin Haygood]

(In reply to comment #19)
> (From update of attachment 5290 [details] [edit])
> Using the secure version is OK, although it doesn't help with the issue that
> "_r" deals with, but why use the 64-bit version? The parameter is just a
> time_t, so there's no benefit to using the 64-bit one.
> 
> The basic concept of the changes to work on Windows are good, but lets not land
> this with the "64" variant.
> 

According to Visual Studio 2005:

time_t is a typedef for time64_t
gmtime(_Time) is an inline function that calls _gmtime64(_Time)

The actual code in the CRT is:

_CRT_INSECURE_DEPRECATE(gmtime_s) static __inline struct tm * __CRTDECL
gmtime(const time_t * _Time)
{
#pragma warning( push )
#pragma warning( disable : 4996 )
    return _gmtime64(_Time);
#pragma warning( pop )
}

[Dave Hyatt]

I have a patch that eliminates the use of _r functions and that makes an inline
gmtoffset function (so that call sites don't have to be #ifdefed).

[Justin Haygood]

That would be good.

What would be the solution for Windows not supporting negative clock values (it
crashes anytime you do gmtime/localtime(-1) or less)

[Maciej Stachowiak]

Justin's patch actually looks right to me. Using localtime() unconditionally is clearly wrong because it can't 
be assumed all platforms use per-thread storage. This is not true on Linux, to cite one example. And even 
if it is true on Mac OS X, it is not even documented as such in the man page so it seems poor to rely on 
this. The _s versions do in fact solve the same problem as the _r versions, they write into a provided struct 
tm instead of using a global one. And using the _r version seems better than hitting per-thread storage 
even on platforms that do have a thread-safe localtime().