Bug 11917 - setlocale() can return null
Summary: setlocale() can return null
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Other Other
: P2 Normal
Assignee: David Carson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-21 10:27 PST by David Carson
Modified: 2007-01-07 21:33 PST (History)
2 users (show)

See Also:


Attachments
patch to CString (1.60 KB, patch)
2006-12-21 14:26 PST, David Carson
no flags Details | Formatted Diff | Diff
Patch v2 (1.20 KB, patch)
2007-01-07 21:14 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Carson 2006-12-21 10:27:32 PST
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
Comment 1 David Carson 2006-12-21 14:26:18 PST
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.
Comment 2 Darin Adler 2006-12-22 17:48:36 PST
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
Comment 3 Darin Adler 2006-12-22 17:49:48 PST
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.
Comment 4 David Kilzer (:ddkilzer) 2007-01-07 21:08:30 PST
(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.)

Comment 5 David Kilzer (:ddkilzer) 2007-01-07 21:10:05 PST
Better link:  http://man.cx/setlocale%283C%29

Comment 6 David Kilzer (:ddkilzer) 2007-01-07 21:14:12 PST
Created attachment 12292 [details]
Patch v2

Remove dead code.
Comment 7 David Kilzer (:ddkilzer) 2007-01-07 21:14:50 PST
Comment on attachment 11956 [details]
patch to CString

Clearing darin's r+ flag.
Comment 8 Darin Adler 2007-01-07 21:21:52 PST
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.
Comment 9 David Kilzer (:ddkilzer) 2007-01-07 21:33:31 PST
Committed revision 18658.