Bug 11917 – setlocale() can return null

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Carson]

According to the spec, 
http://www.opengroup.org/onlinepubs/007908799/xsh/setlocale.html
setlocale() can return null, and it does in some environments. We should check full null when it is being used in date_object.cpp

[David Carson]

Created attachment 11956 [details]
patch to CString

Took the approach that CString should support holding null char pointers.
Had no luck with 24fun JSBench giving consistant results with no changes, so the performance impact of this change I am not sure of.
Would love feedback from a non-mac port as setlocale() is only used in non-darwin builds.

[Darin Adler]

Comment on attachment 11956 [details]
patch to CString

+    length = data = c;

We don't normally do multiple assignments on a line like this, and I think it's a little strange to set length to a null pointer to zero it. Instead it should be "length = 0; data = 0;" on two separate lines.

Otherwise, looks great. r=me

[Darin Adler]

I'm a little concerned about under what circumstances setlocale() would be returning NULL. As far as I know, it only does that when it fails to change the locale, so I think there'd need to be handling for that case other than just leaving the locale as-is.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> I'm a little concerned about under what circumstances setlocale() would be
> returning NULL. As far as I know, it only does that when it fails to change the
> locale, so I think there'd need to be handling for that case other than just
> leaving the locale as-is.

Here's a Linux man page for setlocale():  http://man.cx/setlocale(3C)

According to this man page, calling setlocale() with a NULL locale (second argument) does not change the locale, but simply returns the current one.  Since the oldlocale variable in DateProtoFunc::callAsFunction() in JavaScriptCore/kjs/date_object.cpp is never used again, I'm not sure that code actually does anything.

Doing more research, this code originates from r6 (yes, revision 6) when source from kde-2.2 was imported into the then-CVS repository.  The original call to setlocale that actually changed the locale and the code that changed the locale back has long since been removed.  (See "svn ann -r1023 JavaScriptCore/kjs/date_object.cpp" for enlightenment.)

[David Kilzer (:ddkilzer)]

Better link:  http://man.cx/setlocale%283C%29

[David Kilzer (:ddkilzer)]

Created attachment 12292 [details]
Patch v2

Remove dead code.

[David Kilzer (:ddkilzer)]

Comment on attachment 11956 [details]
patch to CString

Clearing darin's r+ flag.

[Darin Adler]

Comment on attachment 12292 [details]
Patch v2

r=me; clearly the best short term solution to this problem

On the other hand, the code probably meant to set the locale to "" or "C" or "POSIX" -- maybe some day we'll find ourselves adding code to do that. We'll need to test date-related tests on platforms other than Mac OS X with the current locale set to exotic ones to find out if that is needed.

[David Kilzer (:ddkilzer)]

Committed revision 18658.