11917 2006-12-21 10:27:32 -0800 setlocale() can return null 2007-01-07 21:33:31 -0800 1 1 1 Unclassified WebKit JavaScriptCore 420+ Other Other RESOLVED FIXED P2 Normal --- 1 dacarson dacarson darin ddkilzer oldest_to_newest 40763 0 dacarson 2006-12-21 10:27:32 -0800 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 40732 1 11956 dacarson 2006-12-21 14:26:18 -0800 Created attachment 11956 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. 40273 2 11956 darin 2006-12-22 17:48:36 -0800 Comment on attachment 11956 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 40274 3 darin 2006-12-22 17:49:48 -0800 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. 36413 4 ddkilzer 2007-01-07 21:08:30 -0800 (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.) 36414 5 ddkilzer 2007-01-07 21:10:05 -0800 Better link: http://man.cx/setlocale%283C%29 36417 6 12292 ddkilzer 2007-01-07 21:14:12 -0800 Created attachment 12292 Patch v2 Remove dead code. 36418 7 11956 ddkilzer 2007-01-07 21:14:50 -0800 Comment on attachment 11956 patch to CString Clearing darin's r+ flag. 36366 8 12292 darin 2007-01-07 21:21:52 -0800 Comment on attachment 12292 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. 36370 9 ddkilzer 2007-01-07 21:33:31 -0800 Committed revision 18658. 11956 2006-12-21 14:26:18 -0800 2007-01-07 21:14:50 -0800 patch to CString setlocale.txt text/plain 1643 dacarson SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4Mzc3KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMDYtMTItMjEgIGRhY2Fyc29u ICBkYWNhcnNvbkBnbWFpbC5jb20KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworCUZpeCBmb3I6IGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEx OTE3CisgICAgICAgIENoYW5nZSBDU3RyaW5nIHRvIHN1cHBvcnQgTlVMTCBwb2ludGVycy4gVGhp cyBpcyB0aGUgY2FzZQorICAgICAgICB3aGVuIHNldGxvY2FsZSgpIGlzIGNhbGxlZCBpbiBkYXRl X29iamVjdC5jcHAsIHNldGxvY2FsZSgpCisgICAgICAgIGNhbiByZXR1cm4gTlVMTCwgaXQncyBy ZXR1cm4gdmFsdWUgaXMgc3RvcmVkIGluIENTdHJpbmcuCisKKyAgICAgICAgKiBranMvdXN0cmlu Zy5jcHA6CisgICAgICAgIChLSlM6OkNTdHJpbmc6OkNTdHJpbmcpOgorICAgICAgICAoS0pTOjpD U3RyaW5nOjpvcGVyYXRvcj0pOgorCiAyMDA2LTEyLTIwICBBbmRlcnMgQ2FybHNzb24gIDxhY2Fy bHNzb25AYXBwbGUuY29tPgogCiAgICAgICAgICoga2pzL3N0cmluZ19vYmplY3QuY3BwOgpJbmRl eDogSmF2YVNjcmlwdENvcmUva2pzL3VzdHJpbmcuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFTY3Jp cHRDb3JlL2tqcy91c3RyaW5nLmNwcAkocmV2aXNpb24gMTgzNjUpCisrKyBKYXZhU2NyaXB0Q29y ZS9ranMvdXN0cmluZy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUxLDkgKzUxLDEyIEBAIGV4dGVy biBjb25zdCBkb3VibGUgSW5mOwogCiBDU3RyaW5nOjpDU3RyaW5nKGNvbnN0IGNoYXIgKmMpCiB7 Ci0gIGxlbmd0aCA9IHN0cmxlbihjKTsKLSAgZGF0YSA9IG5ldyBjaGFyW2xlbmd0aCsxXTsKLSAg bWVtY3B5KGRhdGEsIGMsIGxlbmd0aCArIDEpOworICBpZiAoYykgeworICAgIGxlbmd0aCA9IHN0 cmxlbihjKTsKKyAgICBkYXRhID0gbmV3IGNoYXJbbGVuZ3RoKzFdOworICAgIG1lbWNweShkYXRh LCBjLCBsZW5ndGggKyAxKTsKKyAgfSBlbHNlCisgICAgbGVuZ3RoID0gZGF0YSA9IGM7CiB9CiAK IENTdHJpbmc6OkNTdHJpbmcoY29uc3QgY2hhciAqYywgc2l6ZV90IGxlbikKQEAgLTEwMSw5ICsx MDQsMTIgQEAgQ1N0cmluZyAmQ1N0cmluZzo6b3BlcmF0b3I9KGNvbnN0IGNoYXIgKgogewogICBp ZiAoZGF0YSkKICAgICBkZWxldGUgW10gZGF0YTsKLSAgbGVuZ3RoID0gc3RybGVuKGMpOwotICBk YXRhID0gbmV3IGNoYXJbbGVuZ3RoKzFdOwotICBtZW1jcHkoZGF0YSwgYywgbGVuZ3RoICsgMSk7 CisgIGlmIChjKSB7CisgICAgbGVuZ3RoID0gc3RybGVuKGMpOworICAgIGRhdGEgPSBuZXcgY2hh cltsZW5ndGgrMV07CisgICAgbWVtY3B5KGRhdGEsIGMsIGxlbmd0aCArIDEpOworICB9IGVsc2Ug CisgICAgbGVuZ3RoID0gZGF0YSA9IGM7CiAKICAgcmV0dXJuICp0aGlzOwogfQo= 12292 2007-01-07 21:14:12 -0800 2007-01-07 21:21:52 -0800 Patch v2 bug-11917-v2.diff text/plain 1225 ddkilzer SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4NjU1KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMDctMDEtMDcgIERhdmlkIEtp bHplciAgPGRka2lsemVyQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgLSBmaXggaHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTE5MTcKKyAgICAgICAgICBzZXRsb2NhbGUoKSBjYW4gcmV0dXJuIG51bGwKKwor ICAgICAgICAqIGtqcy9kYXRlX29iamVjdC5jcHA6CisgICAgICAgIChLSlM6OkRhdGVQcm90b0Z1 bmM6OmNhbGxBc0Z1bmN0aW9uKTogUmVtb3ZlZCBkZWFkIGNvZGUuCisKIDIwMDctMDEtMDQgIERh dmlkIEtpbHplciAgPGRka2lsemVyQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkg S2V2aW4gTWNDdWxsb3VnaC4KSW5kZXg6IEphdmFTY3JpcHRDb3JlL2tqcy9kYXRlX29iamVjdC5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gSmF2YVNjcmlwdENvcmUva2pzL2RhdGVfb2JqZWN0LmNwcAkocmV2 aXNpb24gMTg2NTUpCisrKyBKYXZhU2NyaXB0Q29yZS9ranMvZGF0ZV9vYmplY3QuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC00NTEsMTIgKzQ1MSw2IEBAIEpTVmFsdWUgKkRhdGVQcm90b0Z1bmM6OmNh bGxBc0Z1bmN0aW9uKEUKIAogICBKU1ZhbHVlICpyZXN1bHQgPSAwOwogICBVU3RyaW5nIHM7Ci0j aWYgIVBMQVRGT1JNKERBUldJTikKLSAgQ1N0cmluZyBvbGRsb2NhbGUgPSBzZXRsb2NhbGUoTENf VElNRSwgMCk7Ci0gIGlmICghb2xkbG9jYWxlLnNpemUoKSkKLSAgICBvbGRsb2NhbGUgPSBzZXRs b2NhbGUoTENfQUxMLCAwKTsKLSAgLy8gRklYTUU6IFdoZXJlJ3MgdGhlIGNvZGUgdG8gc2V0IHRo ZSBsb2NhbGUgYmFjayB0byBvbGRsb2NhbGU/Ci0jZW5kaWYKICAgSlNWYWx1ZSAqdiA9IHRoaXNE YXRlT2JqLT5pbnRlcm5hbFZhbHVlKCk7CiAgIGRvdWJsZSBtaWxsaSA9IHYtPnRvTnVtYmVyKGV4 ZWMpOwogICBpZiAoaXNOYU4obWlsbGkpKSB7Cg==