Bug 113168 – LayoutTests/fast/js/large-expressions.html crashes on Linux

• Status: UNCONFIRMED
• Component: JavaScriptCore

[linzj]

LayoutTests/fast/js/large-expressions.html results in crash because buffer overflowed when JSC is running in non-main thread.
This is because the logic which StackBounds::initialize sets stack base parameter has bug.
No matter Linux's glibc or bionic(android's libc)'s implementation configures the newly-being-created thread's stack to having guarded pages in the stack bottom(or the begin of stack memory region).The following is the source from allocatestack.c in glibc:
	  mem = mmap (NULL, size, prot,
		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
          ...
	  char *guard = mem;
          ...
          	  if (mprotect (guard, guardsize, PROT_NONE) != 0)
          ...
And the following is the source from pthread.c in bionic's implementation:
static void *mkstack(size_t size, size_t guard_size)
{
    void * stack;

    pthread_mutex_lock(&mmap_lock);

    stack = mmap(NULL, size,
                 PROT_READ | PROT_WRITE,
                 MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE,
                 -1, 0);

    if(stack == MAP_FAILED) {
        stack = NULL;
        goto done;
    }

    if(mprotect(stack, guard_size, PROT_NONE)){
        munmap(stack, size);
        stack = NULL;
        goto done;
    }

It's clear that the bottom of a pthread's stack is pieces of guarded pages.My patch will fix this bug.

[linzj]

Created attachment 194781 [details]
patch

[WebKit Review Bot]

Attachment 194781 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/StackBounds.cpp']" exit_code: 1
Source/WTF/wtf/StackBounds.cpp:153:  guard_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/StackBounds.cpp:154:  Missing space after ,  [whitespace/comma] [3]
Source/WTF/wtf/StackBounds.cpp:159:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 194781 [details]
patch

Attachment 194781 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17135849

[Early Warning System Bot]

Comment on attachment 194781 [details]
patch

Attachment 194781 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17323040

[Early Warning System Bot]

Comment on attachment 194781 [details]
patch

Attachment 194781 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17147781

[WebKit Review Bot]

Comment on attachment 194781 [details]
patch

Attachment 194781 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17202999

[linzj]

Created attachment 194782 [details]
patch after style fixed

[linzj]

Created attachment 194783 [details]
patch after compilation error fixed

So embarrassing.

[Geoffrey Garen]

Comment on attachment 194783 [details]
patch after compilation error fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=194783&action=review

> Source/WTF/wtf/StackBounds.cpp:149
> +    // no matter in glibc or bionic(android's libc) implementation,

Let's change this comment to: "glibc and bionic (Android's libc) use the guard size API to set a guard page at the stack base."

> Source/WTF/wtf/StackBounds.cpp:154
> +    int rc = pthread_attr_getguardsize(&sattr, &guardSize);

pthread_attr_getguardsize is standard POSIX, so I think we should do this on all platforms.

> Source/WTF/wtf/StackBounds.cpp:155
> +    stackBase = reinterpret_cast<void *>(reinterpret_cast<unsigned long>(stackBase) + guardSize);

"void*" instead of "void *", please.

Also, to cast a pointer to integer, use uintptr_t.

Also, you need to round guardSize up to the system page size.

[linzj]

Created attachment 194976 [details]
patch

I insist that the OS(LINUX) macro should remains in the source.At lease freebsd's pthread implementation has not been checked.

[linzj]

WHEN ARE YOU GOING TO REVIEW?
(In reply to comment #9)
> (From update of attachment 194783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194783&action=review
> 
> > Source/WTF/wtf/StackBounds.cpp:149
> > +    // no matter in glibc or bionic(android's libc) implementation,
> 
> Let's change this comment to: "glibc and bionic (Android's libc) use the guard size API to set a guard page at the stack base."
> 
> > Source/WTF/wtf/StackBounds.cpp:154
> > +    int rc = pthread_attr_getguardsize(&sattr, &guardSize);
> 
> pthread_attr_getguardsize is standard POSIX, so I think we should do this on all platforms.
> 
> > Source/WTF/wtf/StackBounds.cpp:155
> > +    stackBase = reinterpret_cast<void *>(reinterpret_cast<unsigned long>(stackBase) + guardSize);
> 
> "void*" instead of "void *", please.
> 
> Also, to cast a pointer to integer, use uintptr_t.
> 
> Also, you need to round guardSize up to the system page size.

[Allan Sandfeld Jensen]

(In reply to comment #10)
> Created an attachment (id=194976) [details]
> patch
> 
> I insist that the OS(LINUX) macro should remains in the source.At lease freebsd's pthread implementation has not been checked.

Let's check it then, and make sure this only needs to be fixed once.

[Allan Sandfeld Jensen]

Comment on attachment 194976 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194976&action=review

> Source/WTF/wtf/StackBounds.cpp:152
> +    // FIXME: Not sure if the libc of other unix is using the same implementation as linux does.If anyone does,please remove the OS(LINUX) macro.
> +#if OS(LINUX)

pthread_attr_getguardsize is a generic POSIX call, please enable it for all UNIX. Whether this code is correct or not only depends on whether the stack grows up or down, but we seem to only handle stacks growing dow here.

> Source/WTF/wtf/StackBounds.cpp:161
> +    stackBase = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(stackBase) + guardSize);
> +    stackSize -= guardSize;

GuardSize does not affect stackSize. This hack is only necessary because of how m_origin is set lower down. Remove it and just set
m_bound as stackBase + guardSize.

[linzj]

Created attachment 196771 [details]
patch after remove platform macro & move the hack to m_bound

[linzj]

Created attachment 196772 [details]
patch after remove platform macro & move the hack to m_bound

[Allan Sandfeld Jensen]

(In reply to comment #15)
> Created an attachment (id=196772) [details]
> patch after remove platform macro & move the hack to m_bound

Thanks. I may however have been a bit hasty in saying we don't need an ifdef for Linux.There seems to be a difference in opinion on whether guardsize is included or not in stacksize.
http://sourceware.org/ml/libc-help/2008-07/msg00076.html
https://bugzilla.redhat.com/show_bug.cgi?id=435337
http://www.sourceware.org/ml/libc-alpha/2008-05/msg00086.html

More worryingly it may get fixed in new versions of glibc
http://sourceware.org/bugzilla/show_bug.cgi?id=6973
http://sourceware.org/bugzilla/show_bug.cgi?id=11787

None of this matters to setting m_bound which should be correct now, but we may end up setting m_origin too short.

I suggest ifdeffing for GLIBC and in that case subtract guardsize from stacksize like you did the previous patch. m_origin can then be redefined as stackbase + stacksize + guardsize. Btw feel free to typecast with static_cast<char*>, it is shorter than the double reinterpret case.

[linzj]

Created attachment 196832 [details]
patch add glibc & android test

[Allan Sandfeld Jensen]

Comment on attachment 196832 [details]
patch add glibc & android test

View in context: https://bugs.webkit.org/attachment.cgi?id=196832&action=review

Looks good. I however only meant we should subtract guardSize from stackSize for glibc and android. We should still support guardsize for the other cases. It just needs to be added to both m_bound and m_origin.

> Source/WTF/wtf/StackBounds.cpp:153
> +#ifdef __GLIBC__
> +#if __GLIBC__ == 2 && __GLIBC_MINOR__ <= 15

Combine into one line.

> Source/WTF/wtf/StackBounds.cpp:160
> +#if USE(GUARD_SIZE)

I think we should try make the patch general and support guardsize everywhere. It just only needs to be subtracted from stack_size on glibc (and android).

> Source/WTF/wtf/StackBounds.cpp:168
> +    stackBase = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(stackBase) + guardSize);

use static_cast<char*> so you only need one cast.

[linzj]

Created attachment 196980 [details]
patch

[Allan Sandfeld Jensen]

Comment on attachment 196980 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196980&action=review

> Source/WTF/wtf/StackBounds.cpp:158
>      m_bound = stackBase;
>      m_origin = static_cast<char*>(stackBase) + stackSize;

m_bound = stackBase + guardSize
m_origin = stackBase + guardSize + stackSize

> Source/WTF/wtf/StackBounds.cpp:165
> +    m_bound = reinterpret_cast<char*>(m_bound) + guardSize;
> +    stackSize -= guardSize;

m_origin = static_cast<char*>(m_origin) - guardSize;

[linzj]

Created attachment 197463 [details]
patch

[Allan Sandfeld Jensen]

Comment on attachment 197463 [details]
patch

That should work to the best of my knowledge. Not sure about GLIBC_MINOR <= 15 though. We might have to maintain that if it doesn't get fixed.

[linzj]

(In reply to comment #22)
> (From update of attachment 197463 [details])
> That should work to the best of my knowledge. Not sure about GLIBC_MINOR <= 15 though. We might have to maintain that if it doesn't get fixed.

That number is the version number of my glibc,running in an Ubuntu 12.10 machine.

[Allan Sandfeld Jensen]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 197463 [details] [details])
> > That should work to the best of my knowledge. Not sure about GLIBC_MINOR <= 15 though. We might have to maintain that if it doesn't get fixed.
> 
> That number is the version number of my glibc,running in an Ubuntu 12.10 machine.

Okay, then I would suggest removing the version check before landing. We shouldn't introduce a version check until we know glibc has closed the related bug. 
http://sourceware.org/bugzilla/show_bug.cgi?id=6973

[Allan Sandfeld Jensen]

Btw, I seems WinCairo, Qt and GTK+ all have this test disabled. I am going to make it block the respective bugs. It could be this fixes it.

[Allan Sandfeld Jensen]

(In reply to comment #25)
> Btw, I seems WinCairo, Qt and GTK+ all have this test disabled. I am going to make it block the respective bugs. It could be this fixes it.

Well, obviously not WinCairo.

[Zan Dobersek]

Is checking for the Android libc still relevant given the Chromium port has now forked into Blink? Does any of the remaining ports actually support building with the Android libc?

[Allan Sandfeld Jensen]

(In reply to comment #27)
> Is checking for the Android libc still relevant given the Chromium port has now forked into Blink? Does any of the remaining ports actually support building with the Android libc?

Yes, Qt does.

[Zan Dobersek]

The r+ed patch still requires a changelog. I'm mainly interested how, if at all, this patch would affect a couple of old stack-related problems on the GTK port as well as the current set of regressions on the GTK 64-bit release builder after r147892.
http://trac.webkit.org/changeset/147892

[Allan Sandfeld Jensen]

Comment on attachment 197463 [details]
patch

Missing ChangeLog

[linzj]

I will catch up later,in the middle of something.
(In reply to comment #30)
> (From update of attachment 197463 [details])
> Missing ChangeLog

[EWS]

[e-mail] does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 403335 [details] from commit queue.