Bug 12378 – QString::mid is broken (KWQString.mm)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Holger Freyther]

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

[David Kilzer (:ddkilzer)]

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.)

[David Kilzer (:ddkilzer)]

This bug was introduced in r3344 when the QString::left(), right() and mid() functions were rewritten.

http://trac.webkit.org/projects/webkit/changeset/3344

[zalan]

Created attachment 12643 [details]
S60 patch on QString::mid()

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 12643 [details]
S60 patch on QString::mid()

Patch looks fine. r=me

[Bradley Morrison]

(In reply to comment #6)
> (From update of attachment 12643 [details] [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.

[David Kilzer (:ddkilzer)]

(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.

[Darin Adler]

(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.