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.
Created attachment 194781 [details] patch
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.
Comment on attachment 194781 [details] patch Attachment 194781 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17135849
Comment on attachment 194781 [details] patch Attachment 194781 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17323040
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
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
Created attachment 194782 [details] patch after style fixed
Created attachment 194783 [details] patch after compilation error fixed So embarrassing.
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.
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.
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.
(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.
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.
Created attachment 196771 [details] patch after remove platform macro & move the hack to m_bound
Created attachment 196772 [details] patch after remove platform macro & move the hack to m_bound
(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.
Created attachment 196832 [details] patch add glibc & android test
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.
Created attachment 196980 [details] patch
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;
Created attachment 197463 [details] patch
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.
(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.
(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
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.
(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.
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?
(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.
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
Comment on attachment 197463 [details] patch Missing ChangeLog
I will catch up later,in the middle of something. (In reply to comment #30) > (From update of attachment 197463 [details]) > Missing ChangeLog
realsat35@gmail.com 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.