Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault in WTF::StringImpl::copyChars #562

Closed
The-Compiler opened this Issue Jun 11, 2017 · 32 comments

Comments

Projects
None yet
3 participants
@The-Compiler
Copy link
Collaborator

The-Compiler commented Jun 11, 2017

Sometimes when visiting https://time.com/3951006/facebook-visual-recognition/ with the Alpha, and always when writing a comment on Reddit (I can suggest /r/test) with qutebrowser or Otter Browser, I get a crash here:

#0  0x00007fffed413b65 in WTF::StringImpl::copyChars<char16_t>(char16_t*, char16_t const*, unsigned int) (destination=0x7ffefa9e0250 u"&lt;p ", source=0x7fff1baf0256 u"&lt;p class=\"parent\"&gt;&lt;a name=\"dircqkb\"&gt;&lt;/a&gt;&lt;/p&gt;&lt;div class=\"midcol likes\" &gt;&lt;div class=\"arrow upmod login-required access-required\" data-event-action=\"upvote\" role=\"button\""..., numCharacters=20) at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/WTF/wtf/text/StringImpl.h:634
#1  0x00007fffed420926 in JSC::jsSpliceSubstringsWithSeparators (separatorCount=251, separators=<optimized out>, rangeCount=<optimized out>, substringRanges=<optimized out>, source=..., sourceVal=<optimized out>, exec=0x7fffffffc6f0, this=<optimized out>)
    at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/JavaScriptCore/runtime/StringPrototype.cpp:431
#2  0x00007fffed420926 in JSC::replaceUsingRegExpSearch (replaceValue=..., replacementString=..., callType=<optimized out>, callData=..., searchValue=..., string=<optimized out>, exec=<optimized out>)
    at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/JavaScriptCore/runtime/StringPrototype.cpp:666
#3  0x00007fffed420926 in JSC::replaceUsingRegExpSearch (replaceValue=..., searchValue=..., string=<optimized out>, exec=<optimized out>) at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/JavaScriptCore/runtime/StringPrototype.cpp:708
#4  0x00007fffed420926 in JSC::replace(JSC::ExecState*, JSC::JSString*, JSC::JSValue, JSC::JSValue) (replaceValue=..., searchValue=..., string=<optimized out>, exec=<optimized out>) at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/JavaScriptCore/runtime/StringPrototype.cpp:829
#5  0x00007fffed420926 in JSC::replace(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::JSValue) (replaceValue=..., searchValue=..., thisValue=..., exec=<optimized out>) at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/JavaScriptCore/runtime/StringPrototype.cpp:841
#6  0x00007fffed420926 in JSC::stringProtoFuncReplace(JSC::ExecState*) (exec=<optimized out>) at /tmp/makepkg/qt5-webkit-ng-debug/src/webkit-qtwebkit-5.212.0-alpha/Source/JavaScriptCore/runtime/StringPrototype.cpp:846
#7  0x00007fff1bfff0c8 in  ()
#8  0x00007fffffffc7b0 in  ()
#9  0x00007fffed5c930d in llint_entry () at /usr/lib/libQt5WebKit.so.5

thread apply all bt full

From IRC:

15:03 <annulen> The-Compiler: maybe you can bisect?
15:05 <annulen> it should be in 051a9d542f9f0..9439737eff7

haven't had time to try that yet.

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 11, 2017

I can reproduce it with MiniBrowser on the current qtwebkit-5.212 branch.

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 11, 2017

Crash on Reddit doesn't reproduce here. Probably it's because of gcc 7 or Arch-specific environment

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 11, 2017

Building with gcc 7 now

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 11, 2017

I can also reproduce it on 9439737 - trying with a freshly built TP5 now, and will try to bisect further tomorrow if it doesn't reproduce with TP5.

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 11, 2017

BTW, I've reproduced linker error with gcc 7 (on x86_64)

@annulen annulen added the bug label Jun 11, 2017

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

I also got a (different?) linker error when trying to build TP5:

[ 96%] Linking CXX shared library ../../lib/libQt5WebKit.so
../../lib/libWebCore.a(../../lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/ScrollableArea.cpp.o):ScrollableArea.cpp:function WebCore::ScrollableArea::scrollAnimator() const: error: undefined reference to 'WebCore::ScrollAnimator::create(WebCore::ScrollableArea&)'
collect2: error: ld returned 1 exit status
make[2]: *** [Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/build.make:328: bin/TestWebKitAPI/WebCore/TestWebCore] Error 1
make[1]: *** [CMakeFiles/Makefile2:2635: Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
../../lib/libWebCore.a(../../lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/ScrollableArea.cpp.o):ScrollableArea.cpp:function WebCore::ScrollableArea::scrollAnimator() const: error: undefined reference to 'WebCore::ScrollAnimator::create(WebCore::ScrollableArea&)'
collect2: error: ld returned 1 exit status
make[2]: *** [Source/WebKit/CMakeFiles/WebKit.dir/build.make:2662: lib/libQt5WebKit.so.5.602.3] Error 1
make[1]: *** [CMakeFiles/Makefile2:1485: Source/WebKit/CMakeFiles/WebKit.dir/all] Error 2
make: *** [Makefile:163: all] Error 2

but I just tried with Archlinux' legacy QtWebKit package (the one coming with Qt 5.9), and I can reproduce this there as well - so this indeed seems like some GCC 7 thing rather than a regression...

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

When I paste this monster into the inspector, I get the same crash:

s = ';p class="parent";;a name="disiq1u";;/a;;/p;;div class="midcol likes" ;;div class="arrow upmod login-required access-required" data-event-action="upvote" role="button" aria-label="upvote" tabindex="0" ;;/div;;div class="arrow down login-required access-required" data-event-action="downvote" role="button" aria-label="downvote" tabindex="0" ;;/div;;/div;;div class="entry likes";;p class="tagline";;a href="javascript:void(0)" class="expand" onclick="return togglecomment(this)";[–];/a;;a href="https://www.reddit.com/user/The-Compiler" class="author may-blank id-t2_bhpe6" ;The-Compiler;/a;;span class="userattrs";;/span;;#32;;span class="score dislikes" title="-1";-1 points;/span;;span class="score unvoted" title="0";0 points;/span;;span class="score likes" title="1";1 point;/span;;#32;;time title="Mon Jun 12 11:54:37 2017 UTC" datetime="2017-06-12T11:54:37+00:00" class="live-timestamp";1 hour ago;/time;;time class="edited-timestamp" title="last edited just now" datetime="2017-06-12T13:18:13+00:00";*;/time;;nbsp;;a href="javascript:void(0)" class="numchildren" onclick="return togglecomment(this)";(0 children);/a;;/p;;form action="#" class="usertext warn-on-unload" onsubmit="return post_form(this, \'editusertext\')" id="form-t1_disiq1u1ml";;input type="hidden" name="thing_id" value="t1_disiq1u"/;;div class="usertext-body may-blank-within md-container ";;div class="md";;p;Damn, should;#39;ve tried the debug build!;/p;\n\n;p;edit: What about an edit? Blub. Now with even more working JS hackery.;/p;\n;/div;\n;/div;;div class="usertext-edit md-container" style="display: none";;div class="md";;textarea rows="1" cols="1" name="text" class="" ;Damn,;#32;should\'ve;#32;tried;#32;the;#32;debug;#32;build!;#10;;#10;edit:;#32;What;#32;about;#32;an;#32;edit?;#32;Blub.;#32;Now;#32;with;#32;even;#32;more;#32;working;#32;JS;#32;hackery.;/textarea;;/div;;div class="bottom-area";;span class="help-toggle toggle" style="display: none" ;;a class="option active " href="#" tabindex="100" onclick="return toggle(this, helpon, helpoff)" ;formatting help;/a;;a class="option " href="#";hide help;/a;;/span;;a href="/help/contentpolicy" class="reddiquette" target="_blank" tabindex="100";content policy;/a;;span class="error TOO_LONG field-text" style="display:none";;/span;;span class="error RATELIMIT field-ratelimit" style="display:none";;/span;;span class="error NO_TEXT field-text" style="display:none";;/span;;span class="error TOO_OLD field-parent" style="display:none";;/span;;span class="error THREAD_LOCKED field-parent" style="display:none";;/span;;span class="error DELETED_COMMENT field-parent" style="display:none";;/span;;span class="error USER_BLOCKED field-parent" style="display:none";;/span;;span class="error USER_MUTED field-parent" style="display:none";;/span;;span class="error MUTED_FROM_SUBREDDIT field-parent" style="display:none";;/span;;div class="usertext-buttons";;button type="submit" onclick="" class="save" style=\'display:none\';save;/button;;button type="button" onclick="return cancel_usertext(this);" class="cancel" style=\'display:none\';cancel;/button;;span class="status";;/span;;/div;;/div;;div class="markhelp" style="display:none";;p;;p;reddit uses a slightly-customized version of ;a href="http://daringfireball.net/projects/markdown/syntax";Markdown;/a; for formatting. See below for some basics, or check ;a href="/wiki/commenting";the commenting wiki page;/a; for more detailed help and solutions to common issues.;/p;\n\n;/p;;table class="md";;tr style="background-color: #ffff99; text-align: center";;td;;em;you type:;/em;;/td;;td;;em;you see:;/em;;/td;;/tr;;tr;;td;*italics*;/td;;td;;em;italics;/em;;/td;;/tr;;tr;;td;**bold**;/td;;td;;b;bold;/b;;/td;;/tr;;tr;;td;[reddit!](https://reddit.com);/td;;td;;a href="https://reddit.com";reddit!;/a;;/td;;/tr;;tr;;td;* item 1;br/;* item 2;br/;* item 3;/td;;td;;ul;;li;item 1;/li;;li;item 2;/li;;li;item 3;/li;;/ul;;/td;;/tr;;tr;;td;;gt; quoted text;/td;;td;;blockquote;quoted text;/blockquote;;/td;;/tr;;tr;;td;Lines starting with four spaces;br/;are treated like code:;br/;;br/;;span class="spaces";;nbsp;;nbsp;;nbsp;;nbsp;;/span;if 1 * 2 ;lt; 3:;br/;;span class="spaces";;nbsp;;nbsp;;nbsp;;nbsp;;nbsp;;nbsp;;nbsp;;nbsp;;/span;print "hello, world!";br/;;/td;;td;Lines starting with four spaces;br/;are treated like code:;br/;;pre;if 1 * 2 ;lt; 3:;br/;;nbsp'

I can't get it to reproduce with jsc though (need to load from a file, as the line is too long). I bet the inspector does something special with String.replace internally so this happens.

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

I was able to break it down to this, which segfaults in jsc:

s = 'tabindex="0" class="arrow down login-"expand"–'; s.replace(/a/g, 'b')

Almost any change to the string makes the segfault go away.

edit: Some more simplification:

s = 'xxxxxxxxxxxxxxAxxxxxxxxxxxxxxxxxxxxA–'; s.replace(/A/g, 'b')

Note that - at the end is some non-ASCII thing, and removing it makes it go away. Changing the A to something which doesn't match fixes is too. Changing the count of the x in between as well.

That is:

  • 14 chars which don't seem to matter, but need to be there
  • 1 matching char
  • 20 chars like above
  • 1 matching char
  • 1 non-ASCII char
@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

Can you reproduce this crash with Arch package of webkit2gtk built with gcc 7?

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

Looks like the one in the repos is still built with GCC 6. I'll rebuild it with GCC 7, but it's probably going to take me a while.

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

In this case don't bother, it won't compile without patches

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

Segfaults with webkit2gtk 2.16.3 with GCC 7.1.1 too, the stack is a bit different (though I didn't build a debug version):

#0  0x00007ffff7ac2c95 in void WTF::StringImpl::copyChars<char16_t>(char16_t*, char16_t const*, unsigned int) () at /usr/lib/libjavascriptcoregtk-4.0.so.18
#1  0x00007ffff7ac1d5e in JSC::stringProtoFuncReplaceUsingRegExp(JSC::ExecState*) ()
    at /usr/lib/libjavascriptcoregtk-4.0.so.18
#2  0x00007fffb21ff028 in  ()
#3  0x00007fffffffcfc0 in  ()
#4  0x00007ffff77a4e42 in  () at /usr/lib/libjavascriptcoregtk-4.0.so.18
#5  0x0000000000000000 in  ()
@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

Also reproduces with JSC_useJIT=0, JSC_useLLInt=0 and JSC_useRegExpJIT=0.

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

Code:

    template <typename T> static void copyChars(T* destination, const T* source, unsigned numCharacters)
    {
        if (numCharacters == 1) {
            *destination = *source;
            return;
        }

        if (numCharacters <= s_copyCharsInlineCutOff) {
            unsigned i = 0;
#if (CPU(X86) || CPU(X86_64))
            const unsigned charsPerInt = sizeof(uint32_t) / sizeof(T);

            if (numCharacters > charsPerInt) {
                unsigned stopCount = numCharacters & ~(charsPerInt - 1);

                const uint32_t* srcCharacters = reinterpret_cast<const uint32_t*>(source);
                uint32_t* destCharacters = reinterpret_cast<uint32_t*>(destination);
                for (unsigned j = 0; i < stopCount; i += charsPerInt, ++j)
                    destCharacters[j] = srcCharacters[j];
            }
#endif
            for (; i < numCharacters; ++i)
                destination[i] = source[i];
        } else
            memcpy(destination, source, numCharacters * sizeof(T));
    }

it crashes at the destCharacters[j] = srcCharacters[j]; line

What I don't get is this - charsPerInt is calculated as const unsigned charsPerInt = sizeof(uint32_t) / sizeof(T); and is 2 according to gdb, but it tells me sizeof(T) is 4 even though we're using the uint16_t overload?

(gdb) p charsPerInt
$1 = 2
(gdb) p sizeof(T)
$2 = 4
(gdb) p sizeof(uint32_t)
$3 = 4
(gdb) p charsPerInt
$4 = 2
(gdb) p sizeof(uint32_t) / sizeof(T)
$5 = 1

some more debugging:

(gdb) p numCharacters 
$7 = 20
(gdb) p stopCount
$8 = 20
(gdb) p destination
$9 = 0x7fff19281632 u"\xdd53翾"
@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

Valgrind shows no memory erros prior to crash

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

I first thought unsigned stopCount = numCharacters & ~(charsPerInt - 1); would be wrong, because it's 20 when numCharacters is 20, and I expected it to be 10. But the for loop actually breaks on i (not j), and does i += charsPerInt (so 2 in our case), so that seems fine.

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

I removed that optimized block:

diff --git a/Source/WTF/wtf/text/StringImpl.h b/Source/WTF/wtf/text/StringImpl.h
index 5781c1c08fa..e9d7aab36f9 100644
--- a/Source/WTF/wtf/text/StringImpl.h
+++ b/Source/WTF/wtf/text/StringImpl.h
@@ -622,18 +622,6 @@ public:
 
         if (numCharacters <= s_copyCharsInlineCutOff) {
             unsigned i = 0;
-#if (CPU(X86) || CPU(X86_64))
-            const unsigned charsPerInt = sizeof(uint32_t) / sizeof(T);
-
-            if (numCharacters > charsPerInt) {
-                unsigned stopCount = numCharacters & ~(charsPerInt - 1);
-
-                const uint32_t* srcCharacters = reinterpret_cast<const uint32_t*>(source);
-                uint32_t* destCharacters = reinterpret_cast<uint32_t*>(destination);
-                for (unsigned j = 0; i < stopCount; i += charsPerInt, ++j)
-                    destCharacters[j] = srcCharacters[j];
-            }
-#endif
             for (; i < numCharacters; ++i)
                 destination[i] = source[i];
         } else

and now it seems to work fine... I still don't get what's wrong with it though. A minimal example seems to work:

#include <stdint.h>
#include <stdio.h>

static void copyMem(uint16_t* destination, const uint16_t* source, unsigned numCharacters)
{
    unsigned i = 0;
    const unsigned charsPerInt = sizeof(uint32_t) / sizeof(uint16_t);

    if (numCharacters > charsPerInt) {
        unsigned stopCount = numCharacters & ~(charsPerInt - 1);

        const uint32_t* srcCharacters = reinterpret_cast<const uint32_t*>(source);
        uint32_t* destCharacters = reinterpret_cast<uint32_t*>(destination);
        for (unsigned j = 0; i < stopCount; i += charsPerInt, ++j) {
            destCharacters[j] = srcCharacters[j];
            printf("%d\n", j);
        }
    }
    for (; i < numCharacters; ++i)
        destination[i] = source[i];
}

int main(int argc, char *argv[])
{
    uint16_t src[20];
    uint16_t dst[20] = {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20};

    copyMem(dst, src, 20);
    return 0;

}
@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

Works here too without optimized block. UBsan doesn't show anything in minimal example

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

From the #gcc IRC channel:

22:40 <roxfan> at a guess, T* is not compatible with uint32_t* so you get UB
22:40 <The-Compiler> T is uint16_t
22:41 <roxfan> yep, there we go
22:42 <roxfan> you can't access an uint16_t* as uint32_t*
22:42 <roxfan> it may work in practice but it's UB from the standard's POV
22:43 <roxfan> e.g. https://www.cocoawithlove.com/2008/04/using-pointers-to-recast-in-c-is-bad.html
22:43 <The-Compiler> so I'm guessing the point of that block is just to optimize things by copying 4 bytes at a time... anything I could suggest to the devs other than deleting that block?
22:45 <roxfan> going through an intermediate char* cast might work... or maybe try memcpy/memmove instead (make sure to calculate size correctly)
22:46 <roxfan> since you're moving same-sized elements i'm not sure the loop gives you anything over memmove
22:47 <andreyv> The-Compiler: http://dbp-consulting.com/tutorials/StrictAliasing.html
22:48 <andreyv> This code seems more than useless anyway, a standard memcpy()/memmove() will do much better
22:49 <The-Compiler> yeah, in the else below they actually do a memcpy if it's below some threshold (if numCharacters < s_copyCharsInlineCutOff) (which is 20)
22:49 <roxfan> maybe it's slighly faster on short ranges

links:

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

you can't access an uint16_t* as uint32_t*

Crap, if this pattern is broken, it should break most of optimizations based on fast unaligned access

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

This was introduced in 0cb990b FWIW - at least the other one there is using char, which seems to be okay based on "going through an intermediate char* cast might work..."?

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

Yes, but it was in 2011, so in first approximation it always has been there :)

I'll take a look at asm tomorrow, if there is a clear bug I'll prepare reduced test case

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

Not sure if it needs a more reduced testcase, at least it's easy and 100% reproducable 😆

Mind if I open a bug today (or maybe tomorrow morning) already upstream?

Also, are you okay if I submit a bug to the Archlinux package asking to apply the patch above (removing the optimized version) until this is fixed properly?

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

I meant test case for gcc miscompilation

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

Yes, feel free to open bug in WebKit, or I can do it

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

I meant test case for gcc miscompilation

You probably didn't mean this, but this is not GCC's fault - it looks like WebKit doing something non-standard.

I opened https://bugs.archlinux.org/task/54428 now, will open a WebKit bug tomorrow unless you beat me to it 😉

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Jun 12, 2017

Note that we are building with -fno-strict-aliasing

So, I still think it may be GCC bug

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 12, 2017

Ah, good point - I'll let you investigate further then (thanks!) 😄

@The-Compiler

This comment has been minimized.

Copy link
Collaborator Author

The-Compiler commented Jun 15, 2017

Opening a WebKit build now, let's see what they have to say 😉

edit: Opened https://bugs.webkit.org/show_bug.cgi?id=173407 and added you to the Cc

@mitya57

This comment has been minimized.

Copy link

mitya57 commented Sep 27, 2017

Upstream webkit fixed it in 7175db5, maybe you can cherry-pick it?

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Sep 27, 2017

Yes it will be there soon

@annulen

This comment has been minimized.

Copy link
Owner

annulen commented Oct 14, 2017

Backported as 0caec8a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.