327869 - new Date (1899, 0).toLocaleString() causes abnormal program termination if compiled with VC 8

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Michael Daumling]

Long live Microsoft!

new Date (1899, 0).toLocaleString() calls PRMJ_FormatTime(), and that function uses strftime(). In VC 8, strftime() validates its parameters. PRMJ_FormatTime() sets the year to -1 (year - 1900) and Microsoft treats -1 for a year as an invalid parameter (from the online help for strftime()):

This function validates its parameters. If strDest, format, or timeptr is a null pointer, or if the tm data structure addressed by timeptr is invalid (for example, if it contains out of range values for the time or date), the invalid parameter handler is invoked, as described in Parameter Validation. If execution is allowed to continue, the function returns 0 and sets errno to EINVAL.

And guess what? The default invalid parameter handler terminates the program...

strftime() is still 1900-based. Therefore, it cannot be used to print years prior to 1900 anymore.

Comment 1

[Michael Daumling]

Attachment: Brute-force fix

This is a brute-force fix. If the year is < 1900, the year is replaced with the year 8099, which causes strftime() to print 9999 as the year. Afterwards, the year 9999 is replaced with the real year. Ugly, but works.

Brendan, let me know if you can live with such an ugly fix.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 212531 [details] [diff] [review]
Brute-force fix

Hard to object -- could you comment the 8099 number a bit more?  Thanks.

/be

Comment 3

[Michael Daumling]

Attachment: Same fix with added comments for 8099.

Since Brendan already granted review, this patch is just for the records. Will check in that patch today.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 212759 [details] [diff] [review]
Same fix with added comments for 8099.

>     a.tm_mon = prtm->tm_mon;
>     a.tm_wday = prtm->tm_wday;
>-    a.tm_year = prtm->tm_year - 1900;
>+    /* Use 8099 as the year if the year is < 1900. strftime() adds 1900 */
>+    /* the the year, resulting in 8099 + 1900 = 9999, which is a fine marker. */
>+    a.tm_year = badYear ? 8099 : prtm->tm_year - 1900;

Nit: more consistent comment style would add a blank line, then use a major (multiline) comment:

    /*
     * Major comments look
     * like this.
     */

Note also "the the" typo.

/be

Comment 5

[Michael Daumling]

Attachment: At your service...

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 212763 [details] [diff] [review]
At your service...

>     a.tm_wday = prtm->tm_wday;
>-    a.tm_year = prtm->tm_year - 1900;
>+    /* 
>+     * Use 8099 as the year if the year is < 1900. strftime() adds 1900
>+     * the year, resulting in 8099 + 1900 = 9999, which is a fine marker.
>+     */

I hate to be a nit-picker (well, actually, I don't ;-), but you forgot the blank line before the major comment, and there is still a missing "to" before the undoubled "the".

/be

Comment 7

[Michael Daumling]

Attachment: No problem!

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 212786 [details] [diff] [review]
No problem!

So much for Windows compatibility.  Where is Raymond Chen when we need him?

/be

Comment 9

[Michael Daumling]

Patch checked in (prmjtime.c #3.53)

Comment 10

[Stefan Sitter [:ssitter]]

The check in for this bug has caused Bug 328482.

Instead of the time in local format (e.g. "18:00") "Sun Dec 31 1899 1:00:00 GMT+0100" is now displayed in Lightning and Sunbird event dialog.

After backing out the patch on prmjtime.c everything displays fine again.

Comment 11

[Stefan Sitter [:ssitter]]

(In reply to comment #10)
> The check in for this bug has caused Bug 328482.

Calendar/Sunbird/Lightning uses toLocaleFormat to convert a time into local format.

Before this patch "new Date(0, 0, 0, 13, 14, 15, 0).toLocaleFormat('%H:%M:%S')"
returned "13:14:15".

After this patch "new Date(0, 0, 0, 13, 14, 15, 0).toLocaleFormat('%H:%M:%S')"
returns "Sun Dec 31 1899 13:14:15 GMT+0000".

Apparently the format string passed in is completely ignored now. 
This looks like an error in this patch to me.

Comment 12

[Brendan Eich [:brendan]]

Michael, I backed out your change.  Sorry I missed the obvious use-case that broke.  Reopening.

/be

Comment 13

[Michael Daumling]

My patch was really silly.

Fixing the whole Date.xxLocaleXxx area seems to be pretty tough, especially if you cannot use PRMJ_FormatTime(). Since ECMA says that the toLocaleString(), toLocaleDateString() and toLocaleTimeString() are implementation dependent, why not just treat them as synonyms for toString(), toDateString() and toTimeSTring() for now?

Comment 14

[Erik Fabert]

Page Info uses toLocaleString; if you really can't find a better solution I'd suggest you only fall back to toString if the year is < 1900, that shouldn't hurt Page Info.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Another attempt

Michael, hope you don't mind my intrusion.  I think the basic idea
of mapping the invalid years onto a valid but "fake" year and then string
replace that with the real year after strftime() is done is a good approach.
Note also that years above 9999 also causes termination.

This patch maps invalid years to the interval 9900 + year % 100
which makes 2-digit year format (%y) also work.
The patch also handles multiple occurrences, eg "%Y%Y...".

Brendan, you might want to review bug 395836 at the same time.

Comment 16

[Mats Palmgren (inactive)]

Attachment: a few testcases...

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 281000 [details] [diff] [review]
Another attempt

No need for sr? flag requests in SpiderMonkey.

/be

Comment 18

[Brian Crowder]

What cases do we return invalid date for?  It seems like if I use 99999 as my year, I don't get an invalid date error, but the year return by toLocalFormat comes back as 32767.  Fixable?  What does the spec say for this?

Comment 19

[Mats Palmgren (inactive)]

(In reply to comment #18)
> What cases do we return invalid date for?

PRMJ_FormatTime() will fail when strftime() fails (platform dependent), or
when the supplied buffer is too small to hold the value -- when it fails
toLocaleFormat() will use a fallback format (same result as toString()).

> It seems like if I use 99999 as my year, I don't get an invalid date error,
> but the year return by toLocalFormat comes back as 32767.  Fixable?

"new Date(99999, 0).toLocaleString()"
leads to a call to PRMJ_FormatTime() with prtm->tm_year == 32767,
so the error occurs earlier.

The root casue seems to be that 'tm_year' is a 16-bit field:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/prmjtime.h&rev=3.18&root=/cvsroot&mark=67#56

PRMJ_FormatTime() itself would behave well for 'tm_year' values > 32767.

> What does the spec say for this?

ECMA-262 15.9.5.5 says:
Date.prototype.toLocaleString ( )
  This function returns a string value.
  The contents of the string are implementation-dependent, but are
  intended to represent the Date in the current time zone in a convenient,
  human-readable form that corresponds to the conventions of the host
  environment's current locale.

toLocaleFormat() isn't in any ECMA standard according to MDC:
http://developer.mozilla.org/en/docs/index.php?title=User:Waldo:Date.toLocaleFormat&printable=yes

So, I agree that clamping 99999 to 32767 is bug if the platform's
strftime() and 'struct tm' can handle it...

Anyway, I would prefer to handle that in a separate bug (filed bug 398485).

Comment 20

[Mats Palmgren (inactive)]

Attachment: Regression tests

Regression tests, I'll add some more covering years > 32767 in bug 398485.

Comment 21

[Brian Crowder]

Comment on attachment 283479 [details] [diff] [review]
Regression tests

Letting bc do test-case review.

Comment 22

[Bob Clary [:bc] (inactive)]

Comment on attachment 283479 [details] [diff] [review]
Regression tests

>Index: js/tests/js1_5/Date/toLocaleFormat.js
>===================================================================
>RCS file: /cvsroot/mozilla/js/tests/js1_5/Date/toLocaleFormat.js,v
>retrieving revision 1.4
>diff -p -u -1 -2 -r1.4 toLocaleFormat.js
>--- js/tests/js1_5/Date/toLocaleFormat.js	26 May 2007 00:19:36 -0000	1.4
>+++ js/tests/js1_5/Date/toLocaleFormat.js	3 Oct 2007 19:49:04 -0000
>@@ -232,12 +232,143 @@ reportCompare(expect, actual, 'Date.toLo
> 
> expect = '05';
> actual = date.toLocaleFormat('%y');
> reportCompare(expect, actual, 'Date.toLocaleFormat("%y")');
> 
> expect = '2005';
> actual = date.toLocaleFormat('%Y');
> reportCompare(expect, actual, 'Date.toLocaleFormat("%Y")');
> 
> expect = '%';
> actual = date.toLocaleFormat('%%');
> reportCompare(expect, actual, 'Date.toLocaleFormat("%%")');
>+
>+
>+expect = '1899 99';
>+temp='%Y %y';
>+actual = new Date(0, 0, 0, 13, 14, 15, 0).toLocaleFormat(temp);
>+reportCompare(expect, actual, 'Date.toLocaleFormat("'+temp+'")');
>+
>+expect = '1899189918991899189918991899189918991899189918991899189918991899189918991899189918991899';
>+temp = '%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y';
>+actual = new Date(0, 0, 0, 13, 14, 15, 0).toLocaleFormat(temp);
>+reportCompare(expect, actual, 'Date.toLocaleFormat("'+temp+'")');
>+
>+expect = 'xxx189918991899189918991899189918991899189918991899189918991899189918991899189918991899189918991899';
>+temp = 'xxx%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y';
>+actual = new Date(0, 0, 0, 13, 14, 15, 0).toLocaleFormat(temp);
>+reportCompare(expect, actual, 'Date.toLocaleFormat("'+temp+'")');
>+
>+expect = new Date(0, 0, 0, 13, 14, 15, 0).toString();;

nit: double ;;

>+temp = 'xxxx%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y%Y';
>+actual = new Date(0, 0, 0, 13, 14, 15, 0).toLocaleFormat(temp);
>+reportCompare(expect, actual, 'Date.toLocaleFormat("'+temp+'")');

1777 static JSBool
1778 date_toLocaleHelper(JSContext *cx, const char *format, jsval *vp)
1779 {
1780     JSObject *obj;
1781     char buf[100];

The 100 byte limitation on the output string is SpiderMonkey only right? Can you add a comment 
to that fact?

r+ with those nits picked.

Comment 23

[Mats Palmgren (inactive)]

Attachment: Regression tests

(In reply to comment #22)
> nit: double ;;

fixed

> The 100 byte limitation on the output string is SpiderMonkey only right?

No, that limitation applies to all products AFAICT.
E.g. the following link results in a fallback date string in Firefox:
javascript:alert((new%20Date).toLocaleFormat('1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'))

Comment 24

[Bob Clary [:bc] (inactive)]

(In reply to comment #23)
> Created an attachment (id=284184) [details]
> Regression tests
> 
> (In reply to comment #22)
> > nit: double ;;
> 
> fixed

Ok. thanks.

> 
> > The 100 byte limitation on the output string is SpiderMonkey only right?
> 
> No, that limitation applies to all products AFAICT.

I'm sorry. I wasn't clear. I meant only in Mozilla's implementation rather than in IE or Opera but since neither supports toLocaleFormat it doesn't really matter. I also need to also move this test into extensions as a non-standard feature.

How would I go about testing the two paths in date_toLocaleFormat where JS_FALSE is returned? Also, if I pass an object to toLocaleFormat, it will return the object unchanged. Is that a valid test?

date_toLocaleFormat(JSContext *cx, uintN argc, jsval *vp)
{
    JSString *fmt;
    const char *fmtbytes;

    if (argc == 0)
        return date_toLocaleString(cx, argc, vp);

    fmt = js_ValueToString(cx, vp[2]);
    if (!fmt)
        return JS_FALSE;
    vp[2] = STRING_TO_JSVAL(fmt);
    fmtbytes = js_GetStringBytes(cx, fmt);
    if (!fmtbytes)
        return JS_FALSE;

    return date_toLocaleHelper(cx, fmtbytes, vp);
}

I'll go ahead and check this test in when its ready. I need to segregate some of the commented out tests for bug 395836 along with checking with Norris.

Norris, is this a test you would want excluded or modified for Rhino?

Comment 25

[Bob Clary [:bc] (inactive)]

I've gone ahead and checked this in with some minor modifications including additional credits for Michael and Mats.

Checking in toLocaleFormat.js;
/cvsroot/mozilla/js/tests/js1_5/Date/toLocaleFormat.js,v  <--  toLocaleFormat.js
new revision: 1.5; previous revision: 1.4

I will be moving it to js1_5/extensions/ shortly. See Bug 370585 for details.

Comment 26

[Brian Crowder]

While this isn't a huge bug, I think it's worth fixing and we have what looks like a reasonable and relatively safe patch.  Thus, P3

Comment 27

[Mike Schroepfer]

Easy fix - already has approved patch so just moving to blocking list

Comment 28

[Reed Loden [:reed]]

Checking in js/src/prmjtime.c;
/cvsroot/mozilla/js/src/prmjtime.c,v  <--  prmjtime.c
new revision: 3.64; previous revision: 3.63
done

Comment 29

[Bob Clary [:bc] (inactive)]

js1_5/extensions/toLocaleFormat-01.js verfied fixed 1.9.0 linux|mac|win