Bug 9651 – MSVCRT time functions are too strict

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Björn Graf (boki)]

The time functions found in Microsofts VC++ 2005 CRT implementation are too strict (e.g. localtime does not support negative time values) which cause null pointer deferencing crashes in kjs/date_object.cpp.

This bug report renders bug 8112 obsolete. Not sure about bug 5311 and bug 5313, but the following patch could be easily change to implement the *_r functions instead.

[Björn Graf (boki)]

Created attachment 9093 [details]
Custom implementations of time functions

Fixes for MSVCRT strict time functions (gmtime, localtime, mktime) by adding custom implementations to the KJS namespace, supporting the non-standard tm::tm_gmtoffset. Also features minor code style updates.

[Darin Adler]

Comment on attachment 9093 [details]
Custom implementations of time functions

This seems like too much platform-specific code to put into this file.

I suggest putting it somewhere in WTF instead, maybe a file named <wtf/DateExtras.h>. Maybe we can move the Mac-specific data locale code there too.

+    //if (secs < time_tMin || secs > time_tMax) {

We don't check in commented-out code.

While the reformatting is fine, ideally I'd like to see that in a separate patch rather than in the same patch as a substantive change to the file. Maybe we can get that in first.

I think it's not so great to have a KJS::tm on the Windows platform and a global tm on all the others. Maybe we can use a typedef instead?

[Björn Graf (boki)]

Created attachment 9244 [details]
Custom implementaion of time functions in WTF

This patch moves the implementaion of the custom time functions into the WTF namespace. This requires the callers to specify the namespace for the functions and wrapper functions for other OSes.

No whitespace was harmed during the creation of this patch.

[Darin Adler]

Comment on attachment 9244 [details]
Custom implementaion of time functions in WTF

Looks like a step in the right direction.

But why are all these functions entirely inlined? That doesn't seem right for code as big as this. Lets put the implementation in the .cpp file.

In yearFromTime, it looks to me like the code would be better if instead of calling floor in the year expression the code simply cast to (int). That does the floor "for free" and we want year to be an int anyway. The only uses of it are casted to int later and the only operation done subsequently is to subtract 1.

This looks great. Please fix DateExtras.h to not be all inline, and then lets get that in!

[Björn Graf (boki)]

Ok, the patch has been changed according to Mr. Adlers comments but I discovered that strftime behaves a bit funcky in the toLocale* cases (like all of the CRT functions that depend on some environment variable; should be easy to fix with Windows GetDateFormat/GetTimeFormat).

On a related note, KJS::parseDate() does not seem save for locale date/time string input with its hardcoded english month names - but, Firefox and IE seem to fail Mozillas ecma_3/Date/15.9.5.6.js test on a German system, while Opera 9 succeeded (but its toLocaleDateString returns a different format), so I am not sure if parseDate needs to be fixed...

[Björn Graf (boki)]

Created attachment 9351 [details]
Custom implementaion of time functions in WTF, not inlined

This patch is the same as before with Darins comments applied. New to this patch is the use of wcsftime over strftime and the minimal implementation of wcsftime to support DateProtoFunc::callAsFunction() on Windows. While I am not absolutely sure if the use of wcsftime is better than using strftime and let the convertion from the system code page to Unicode be performed by UString it feels more logical to get the Unicode string and save the conversion.

[Björn Graf (boki)]

Created attachment 9383 [details]
Custom implementaion of time functions in WTF, not inlined 2

Same as 9351 but with a compiler error fix.

[Darin Adler]

Comment on attachment 9383 [details]
Custom implementaion of time functions in WTF, not inlined 2

+    int dfy = dayFromYear((int)year);

no need for type cast

+    if (day < dfy) {
+        // The first day of year is beyond day, so we have right year + 1
+        year -= 1;
     }

no need for braces

-  static double time_tMin = (time_tIsSigned ? - (double)(1ULL << (8 * sizeof(time_t) - 1)) : 0);
+  if (secs < 0 || secs > time_tMax) {

If this is removed, so that mean that time_tIsSigned is not needed any more?

-  char timebuffer[bufsize];
+  wchar_t timebuffer[bufsize];

What's the rationale for this change?

+      size_t len = WTF::wcsftime(timebuffer, bufsize, L"%c", &t);
+      return jsString(UString((UChar*)timebuffer, len));

This assumes that wchar_t is the same as UChar, but that's not going to be true on platforms other than Windows.

+        struct tm t3;

Should be just "tm", not "struct tm", right?

+/*
+*/

What is that empty comment about?

+inline bool getTimeZoneInfo() {
+inline SYSTEMTIME tmToSystemTime(const struct tm& tm) {

We put braces on the next line when defining functions.

+    //FIXME: Cache the result per thread

We put spaces after // comments.

+inline bool operator >=(const struct tm& a, const struct tm& b)
+{
+    bool res = false;
+    if (a.tm_year == b.tm_year) {
+        if (a.tm_mon > b.tm_mon) {
+            res = true;
+        } else if (a.tm_mon == b.tm_mon) {
+            if (a.tm_mday > b.tm_mday) {
+                res = true;
+            } else if (a.tm_mday == b.tm_mday) {
+                if (a.tm_hour > b.tm_hour) {
+                    res = true;
+                } else if (a.tm_hour == b.tm_hour) {
+                    if (a.tm_min >= b.tm_min) {
+                        res = true;
+                    }
+                }
+            }
+        }
+    }
+    return res;

Would read a lot better if it had a few more return statements in it.

+static inline tm* gmtime(const time_t* timer) { return ::gmtime(timer); }
+static inline tm* localtime(const time_t* timer) { return ::localtime(timer); }
+static inline time_t mktime(tm* time) { return ::mktime(time); }
+static inline size_t wcsftime(wchar_t* dest, size_t maxsize, const wchar_t* format, const tm* timeptr) { return ::wcsftime(dest, maxsize, format, timeptr); }

These should not have "static" in front of them -- just inline.

+#ifndef _DATEEXTRAS_H_

Should not use a leading underscore -- that's reserved for the compiler and library.

+#if HAVE(SYS_TIME_H)
+#if PLATFORM(WIN)

Need to include "Platform.h" first if a header file is going to use these macros.

[Björn Graf (boki)]

(In reply to comment #8)
> +    if (day < dfy) {
> +        // The first day of year is beyond day, so we have right year + 1
> +        year -= 1;
>      }
> 
> no need for braces

The WebKit style guidelines are so contrary to my own that I sometimes miss to follow the right ones...

> -  static double time_tMin = (time_tIsSigned ? - (double)(1ULL << (8 *
> sizeof(time_t) - 1)) : 0);
> +  if (secs < 0 || secs > time_tMax) {
> 
> If this is removed, so that mean that time_tIsSigned is not needed any more?

No. time_tIsSigned is used in time_tMax. Actually it might be better to limit time_tMax to the defined valid range of the time functions (1970 - 2038) like it is done in makeTime().

> -  char timebuffer[bufsize];
> +  wchar_t timebuffer[bufsize];
> 
> What's the rationale for this change?

Using wcsftime instead of strftime to save the conversion to Unicode in the code line next to it.

> +      size_t len = WTF::wcsftime(timebuffer, bufsize, L"%c", &t);
> +      return jsString(UString((UChar*)timebuffer, len));
> 
> This assumes that wchar_t is the same as UChar, but that's not going to be true
> on platforms other than Windows.
>

This is interesting: there is no way to do a wcsftime(str.getBuffer(bufsize), bufsize, ...) on non-Windows systems then? At least it explains why there is no UString c'tor that accepts a pointer to wchar_t. I guess I need to revert to strftime then.

> +    //FIXME: Cache the result per thread
> 
> We put spaces after // comments.

VC++ has the nice little feature showing comments like TODO and FIXME in its task list. The drawback is that it does not allow a space between the comment and the token. Anyway, it looks like I forgot to transform them.

> +#ifndef _DATEEXTRAS_H_
> 
> Should not use a leading underscore -- that's reserved for the compiler and
> library.

I always thought that two leading underlines are reserved for the compiler only. Should it be changed to WTF_DATE_EXTRAS_H or KXMLCORE_DATE_EXTRAS_H to match the other header files in WTF?

> +#if HAVE(SYS_TIME_H)
> +#if PLATFORM(WIN)
> 
> Need to include "Platform.h" first if a header file is going to use these
> macros.

Neither AlwaysInline.h nor FastMalloc.h include Platform.h so I assumed it was save to not include platform.h.

[Gavin Barraclough]

Since this bug is 6 years old, and JSC works on Windows and the referenced bugs are all fixed, I think this issue has since been resolved.  If there is still a problem here please feel free to reopen & attach an updated patch.