12378 2007-01-23 02:18:26 -0800 QString::mid is broken (KWQString.mm) 2007-01-24 11:10:09 -0800 1 1 1 Unclassified WebKit WebCore Misc. 418.x Mac OS X 10.4 RESOLVED FIXED P2 Normal --- 1 zecke webkit-unassigned bradley.morrison darin ddkilzer mjs mrowe rachael yongjun.zhang zalan oldest_to_newest 33143 0 zecke 2007-01-23 02:18:26 -0800 QString::mid from KWQ is broken. It refers to index which is not a class member and not a global variable. index is the function from the C library. The address of index is taken and compared to 0. As index is not declared as weak symbol it is very unlikely that this comparsions leads to true. The following code illustrates the issue: cat method_test.mm #include <stdio.h> #include <stdlib.h> #include <strings.h> #include <unistd.h> int main(int argc, char **argv) { if( fork == 0 && index == 0 ) printf("Foo Bar\n"); return EXIT_SUCCESS; } when compiling and executing nothing is printed on the terminal. I have not searched for more occurences of the same pattern (taking an address of a function and comparing it to a value). Both WebKit418 and the Series60 port are affected 33130 1 ddkilzer 2007-01-23 04:46:54 -0800 This was fixed on WebKit ToT (trunk) in r12905 by mjs while porting QString to win32. (The code is now in WebCore/platform/DeprecatedString.cpp.) http://trac.webkit.org/projects/webkit/changeset/12905 This bug still exists on the Safari-2-0-branch. (The code is in WebCore/kwq/KWQString.mm.) Does this need a Radar bug? This bug still exists on S60/trunk. (The code is in WebCore/kwq/KWQString.cpp.) 33098 2 ddkilzer 2007-01-23 05:22:22 -0800 This bug was introduced in r3344 when the QString::left(), right() and mid() functions were rewritten. http://trac.webkit.org/projects/webkit/changeset/3344 32852 3 12643 zalan 2007-01-24 05:03:53 -0800 Created attachment 12643 S60 patch on QString::mid() 32859 4 darin 2007-01-24 09:24:01 -0800 (In reply to comment #1) > This bug still exists on the Safari-2-0-branch. (The code is in > WebCore/kwq/KWQString.mm.) Does this need a Radar bug? Given that the bug has no known symptom, I don't think it's important to fix it for the Safari 2.0 branch. 32860 5 darin 2007-01-24 09:25:11 -0800 This check is a performance optimization for the special case where start is 0 -- that's another reason I think it's not urgent to fix for the Safari 2.0 branch. 32861 6 12643 darin 2007-01-24 09:25:57 -0800 Comment on attachment 12643 S60 patch on QString::mid() Patch looks fine. r=me 32862 7 bradley.morrison 2007-01-24 09:35:55 -0800 (In reply to comment #6) > (From update of attachment 12643 [edit]) > Patch looks fine. r=me > Thanks for the report, patch and the review! Landed in s60 branch (r19077). Setting to resolved as it's fixed in ToT and is not required for safari 2.0. 32863 8 ddkilzer 2007-01-24 09:59:58 -0800 (In reply to comment #5) > This check is a performance optimization for the special case where start is 0 > -- that's another reason I think it's not urgent to fix for the Safari 2.0 > branch. The worst case would be if a "false positive" condition occurred and the method returned early with the wrong results. An analysis of all the code that ends up calling this method would have to be performed, though. 32868 9 darin 2007-01-24 11:10:09 -0800 (In reply to comment #8) > The worst case would be if a "false positive" condition occurred and the method > returned early with the wrong results. An analysis of all the code that ends > up calling this method would have to be performed, though. No, that false positive case doesn't exist. The index symbol can't be 0. 12643 2007-01-24 05:03:53 -0800 2007-01-24 09:25:57 -0800 S60 patch on QString::mid() indexToStart.txt text/plain 1130 zalan SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9DaGFuZ2VM b2cgICAocmV2aXNpb24gMTkwNzUpDQorKysgV2ViQ29yZS9DaGFuZ2VMb2cgICAod29ya2luZyBj b3B5KQ0KQEAgLTEsMyArMSwxMiBAQA0KKzIwMDctMDEtMjQgIGJ1anRhcyAgPHpidWp0YXNAZ21h aWwuY29tPg0KKw0KKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuDQorICAgICAg ICBERVNDOiAnaW5kZXgnIHZhcmlhYmxlIGlzIG5vdCBkZWNsYXJlZCwgY2hhbmdlIGl0IHRvICdz dGFydCcgDQorICAgICAgICBodHRwOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MjM3OA0KKw0KKyAgICAgICAgKiBrd3EvS1dRU3RyaW5nLmNwcDoNCisgICAgICAgIChRU3RyaW5n OjptaWQpOg0KKw0KIDIwMDctMDEtMjIgIHczbGl1ICA8d2VpLmxpdUBub2tpYS5jb20+DQogDQog ICAgICAgICBSZXZpZXdlZCBieSBaYWxhbiBCdWp0YXMgPHpidWp0YXNAZ21haWwuY29tPiwgbGFu ZGVkIGJ5IGJyYWQuDQpJbmRleDogV2ViQ29yZS9rd3EvS1dRU3RyaW5nLmNwcA0KPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQ0KLS0tIFdlYkNvcmUva3dxL0tXUVN0cmluZy5jcHAgICAocmV2aXNpb24gMTkwNzUpDQorKysg V2ViQ29yZS9rd3EvS1dRU3RyaW5nLmNwcCAgICh3b3JraW5nIGNvcHkpDQpAQCAtMTcyNiw3ICsx NzI2LDcgQEAgUVN0cmluZyBRU3RyaW5nOjptaWQodWludCBzdGFydCwgdWludCBsZQ0KICAgICAg ICAgaWYoIGxlbiA+IGRhdGEuX2xlbmd0aCAtIHN0YXJ0ICkNCiAgICAgICAgICAgICBsZW4gPSBk YXRhLl9sZW5ndGggLSBzdGFydDsNCiANCi0gICAgICAgIGlmICggaW5kZXggPT0gMCAmJiBsZW4g PT0gZGF0YS5fbGVuZ3RoICkNCisgICAgICAgIGlmKCBzdGFydCA9PSAwICYmIGxlbiA9PSBkYXRh Ll9sZW5ndGggKQ0KICAgICAgICAgICAgIHJldHVybiAqdGhpczsNCiANCiAgICAgICAgIEFTU0VS VCggc3RhcnQrbGVuPD1kYXRhLl9sZW5ndGggKTsgIC8vIHJhbmdlIGNoZWNrDQo=