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

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
JavaScript Engine
P3
critical
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Michael Daumling, Assigned: mats)

Tracking

Trunk
mozilla1.9beta2
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 212531 [details] [diff] [review]
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.
Attachment #212531 - Flags: review?(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
Attachment #212531 - Flags: review?(brendan) → review+
(Reporter)

Comment 3

12 years ago
Created attachment 212759 [details] [diff] [review]
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.
Attachment #212531 - Attachment is obsolete: true
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
(Reporter)

Comment 5

12 years ago
Created attachment 212763 [details] [diff] [review]
At your service...
Attachment #212763 - Flags: review?(brendan)
(Reporter)

Updated

12 years ago
Attachment #212759 - Attachment is obsolete: true
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
Attachment #212763 - Flags: review?(brendan) → review-
(Reporter)

Comment 7

12 years ago
Created attachment 212786 [details] [diff] [review]
No problem!
Attachment #212763 - Attachment is obsolete: true
Attachment #212786 - Flags: review?(brendan)
Comment on attachment 212786 [details] [diff] [review]
No problem!

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

/be
Attachment #212786 - Flags: review?(brendan) → review+
(Reporter)

Comment 9

12 years ago
Patch checked in (prmjtime.c #3.53)
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 10

12 years ago
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.
Blocks: 328482

Comment 11

12 years ago
(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.
Michael, I backed out your change.  Sorry I missed the obvious use-case that broke.  Reopening.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

12 years ago
Assignee: general → daumling
Status: REOPENED → NEW
(Reporter)

Comment 13

12 years ago
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

12 years ago
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.

Updated

11 years ago
Blocks: 354073
(Assignee)

Comment 15

10 years ago
Created attachment 281000 [details] [diff] [review]
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.
Assignee: daumling → mats.palmgren
Attachment #212786 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #281000 - Flags: superreview?(brendan)
Attachment #281000 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Severity: major → critical
Flags: in-testsuite?
Flags: blocking1.9?
Keywords: crash
(Assignee)

Comment 16

10 years ago
Created attachment 281002 [details]
a few testcases...
Severity: critical → major
Severity: major → critical
Comment on attachment 281000 [details] [diff] [review]
Another attempt

No need for sr? flag requests in SpiderMonkey.

/be
Attachment #281000 - Flags: superreview?(brendan)
Attachment #281000 - Flags: superreview?
Attachment #281000 - Flags: review?(crowder)
Attachment #281000 - Flags: review?(brendan)

Comment 18

10 years ago
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?
(Assignee)

Comment 19

10 years ago
(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).
(Assignee)

Comment 20

10 years ago
Created attachment 283479 [details] [diff] [review]
Regression tests

Regression tests, I'll add some more covering years > 32767 in bug 398485.
Attachment #283479 - Flags: review?(crowder)
(Assignee)

Updated

10 years ago
Attachment #281000 - Flags: superreview?

Comment 21

10 years ago
Comment on attachment 283479 [details] [diff] [review]
Regression tests

Letting bc do test-case review.
Attachment #283479 - Flags: review?(crowder) → review?(bclary)

Updated

10 years ago
Attachment #281000 - Flags: review?(crowder) → review+

Comment 22

10 years ago
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.
Attachment #283479 - Flags: review?(bclary) → review+
(Assignee)

Updated

10 years ago
Attachment #281000 - Flags: approval1.9?
(Assignee)

Comment 23

10 years ago
Created attachment 284184 [details] [diff] [review]
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'))
Attachment #283479 - Attachment is obsolete: true

Comment 24

10 years ago
(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

10 years ago
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.
Flags: in-testsuite? → in-testsuite+

Comment 26

10 years ago
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
Priority: -- → P3

Updated

10 years ago
Attachment #281000 - Flags: approval1.9? → approval1.9+

Updated

10 years ago
Keywords: crash → checkin-needed

Comment 27

10 years ago
Easy fix - already has approved patch so just moving to blocking list
Flags: blocking1.9? → blocking1.9+
Checking in js/src/prmjtime.c;
/cvsroot/mozilla/js/src/prmjtime.c,v  <--  prmjtime.c
new revision: 3.64; previous revision: 3.63
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Comment 29

10 years ago
js1_5/extensions/toLocaleFormat-01.js verfied fixed 1.9.0 linux|mac|win
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.