Bug 24674 – Crashes in !PLATFORM(MAC)'s formatLocaleDate, in very specific situations

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

[Gustavo Noronha (kov)]

While testing JavaScriptCore for the GTK+ port, I found that test ecma_3/Date/15.9.5.6.js was crashing. It was being run with LC_ALL=C and TZ=PST+8. The problem is that JavaScriptCore assumes strftime's %#x format will always generate >= 4-digits years, and the call it does to strstr() fails when 2-year digits are returned.

[Gustavo Noronha (kov)]

Created attachment 28722 [details]
proposed fix

Would be good to see if this is a GNU/Linux specific problem, or if other libcs would also return 2-digits years in those circunstances.

[Gustavo Noronha (kov)]

Created attachment 28723 [details]
backtrace of the crash

[Darin Adler]

Comment on attachment 28722 [details]
proposed fix

> +#include <langinfo.h>

Is langinfo.h available on all the platforms we need to support, such as Windows? If not, then this needs to be inside an #if.

> +        char* yPos = strstr(formatString, "y");

Should be strchr, not strstr.

Is this really the simplest possible solution?

[Gustavo Noronha (kov)]

(In reply to comment #3)
> (From update of attachment 28722 [details] [review])
> > +#include <langinfo.h>
> 
> Is langinfo.h available on all the platforms we need to support, such as
> Windows? If not, then this needs to be inside an #if.

According to http://www.cygwin.com/cygwin-api/compatibility.html#std-susv3, it should be available for windows. Since it's POSIX.1-2001, I'm pretty sure all the unixy platforms will have it. Is there a canonical list of platforms we need to support, by the way?

> Is this really the simplest possible solution?

I have put some thought on the problem, and couldn't find a solution that would be simple, not impact the performance of the common case, and still work for cases where the year needs to be offset. I'm open for ideas to try out, though.

[Gustavo Noronha (kov)]

Created attachment 28729 [details]
proposed fix

Use strchr and correctly const_cast when attributing from formatStrings to formatString.

[Darin Adler]

(In reply to comment #4)
> Since it's POSIX.1-2001, I'm pretty sure all
> the unixy platforms will have it. Is there a canonical list of platforms we
> need to support, by the way?

There's not.

[Thadeu Lima de Souza Cascardo]

Created attachment 28733 [details]
proposed fix

This is more simple and does not present the problems with parseDate either. kov should present some performance tests he made with it.

[Gustavo Noronha (kov)]

While discussing this with cascardo we found out that never allowing strftime to return 2-digit years has no apparent performance impact (I tried sunspider runs and made a small test js script to run toLocaleDate() on a date object with current time, so I'd agree with going that route. The parseDate problem he mentions is because we'd need extra checks for years between 1900 and 1950.

[Gustavo Noronha (kov)]

Created attachment 28735 [details]
proposed fix

Here's a properly formatted patch, including a changelog entry, with the simplified approach. I tested that it fixes the crash and the test passes.

[Darin Adler]

Comment on attachment 28735 [details]
proposed fix

r=me

[Gustavo Noronha (kov)]

Landed as r41818.

[Ada Chan]

r41818 broke the windows build.  The change has been rolled out.

[Mark Rowe (bdash)]

Comment on attachment 28735 [details]
proposed fix

Clearing review flag due to the build breakage this caused.

[Gustavo Noronha (kov)]

Created attachment 28752 [details]
second try

I am not sure why langinfo.h is not found on windows, since cygwin's documentation says it's supported; it may be some build setup, and I'd be interested in having someone from the windows port work with me on this. In the meanwhile, I am proposing the change again, this time wrapped in a check HAVE(LANGINFO_H).

[Adam Roben (:aroben)]

(In reply to comment #14)
> I am not sure why langinfo.h is not found on windows, since cygwin's
> documentation says it's supported;

Apple's Windows port doesn't use Cygwin headers in its build.

[Gustavo Noronha (kov)]

(In reply to comment #15)
> (In reply to comment #14)
> > I am not sure why langinfo.h is not found on windows, since cygwin's
> > documentation says it's supported;
> 
> Apple's Windows port doesn't use Cygwin headers in its build.

Yeah, Ada told me that. I was under this impression because the page about building Webkit mentions cygwin: http://webkit.org/building/build.html. As does the tools page: http://webkit.org/building/tools.html. I think the additional condition is the way to go, in this case.

[Ada Chan]

Tested the latest patch on windows build environment and it built successfully.

[Gustavo Noronha (kov)]

Created attachment 28844 [details]
simplified second try

I modified the patch so that it doesn't duplicate code. I set up a windows build environment and tested that the patch builds, too. Also tested with Qt. The test I originally set out to fix is fixed, too =).

[Adam Roben (:aroben)]

Comment on attachment 28844 [details]
simplified second try

r=me

Bonus points for making sure it works on the other platforms!

[Gustavo Noronha (kov)]

Landed as r41909, now let me fetch some pop corn to watch the bot =).