675922 - cx->stackLimit is a bogus pointer to stack

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

I can reproduce it on i386/RHEL4 system (gcc 4.1), growth direction is -1.

cx->stackLimit a bogus pointer to stack (because of small gMaxStackSize) and it causes assertions like:

too much recursion
Assertion failure: JS_CHECK_STACK_SIZE(cx->stackLimit - (1 << 14), &stackDummy), at jsgc.cpp:2602

(gdb) p	&stackDummy
$1 = (int *) 0xbfe10fc0
(gdb) p	/x cx->stackLimit - (1 << 14)
$4 = 0xbff81ee1
(gdb) p	/x cx->stackLimit
$22 = 0xbff85ee1

But it's okay because the stack begins at 0xc0000000 and grows to 0xbf600000 ( from GetNativeStackBaseImpl(), stack direction is -1):

(gdb) p stackBase
$48 = (void *) 0xbf600000
(gdb) p /x stackSize
$49 = 0xa00000

If the cx->stackLimit + some assumed stack size is supposed to operate on some stack subregion we can't compare it with a general stack variable like the stackDummy.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Would you guys accept a patch where the stack size is get by GetNativeStackStack()? It could return the exact size for unix and some hardcoded value for other systems.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 550830 [details] [diff] [review]
patch

Gets stack size from system and uses it if possible.

Comment 4

[Igor Bukanov]

Comment on attachment 550830 [details] [diff] [review]
patch

Review of attachment 550830 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +2791,5 @@
>      } else {
>          jsuword stackBase = reinterpret_cast<jsuword>(JS_THREAD_DATA(cx)->nativeStackBase);
> +        size_t nativeStackSize = JS_THREAD_DATA(cx)->nativeStackSize;
> +        if(nativeStackSize)
> +            stackSize = nativeStackSize;

The change should bound stackSize by the native size if the latter is known like:

size_t maxSize = nativeStackSize - 8096;
if (stackSize > maxSize)
    stackSize = maxSize;

Where 8096 allows to detect the stack exhausting before we break the real limit. 

Also the code should move this size trimming outside the the if JS_STACK_GROWTH_DIRECTION - it should be common for both #if branches.

::: js/src/jscntxt.h
@@ +203,5 @@
>      /* Base address of the native stack for the current thread. */
>      jsuword             *nativeStackBase;
>  
> +    /* Base address of the native stack for the current thread. */
> +    size_t               nativeStackSize;

Fix the comments and write that this is zero if unknown. Also rename the field into nativeStackSizeOrZero.

::: js/src/jsnativestack.h
@@ +60,5 @@
> +
> +inline size_t
> +GetNativeStackSize()
> +{
> +    return(GetNativeStackSizeImpl());

The style is return expression-without-extra-parenthesis.

Also add the same asserts as in GetNativeStackBase that the stack size is at least-word-aligned.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: v2

Igor, Thanks for the review. This one should address the comments.

(In reply to Igor Bukanov from comment #4)
> The change should bound stackSize by the native size if the latter is known
> like:
> 
> size_t maxSize = nativeStackSize - 8096;
> if (stackSize > maxSize)
>     stackSize = maxSize;

It does not address the main issue when predefined stackSize is too low thus cx->stackLimit point to middle of the stack and the stack overflow check is broken. The check would be:

size_t maxSize = nativeStackSize - 8096;
if (stackSize > maxSize)
    stackSize = maxSize;
if (stackSize < maxSize)
    stackSize = maxSize;

Which is the same as is it now.

Comment 6

[Igor Bukanov]

(In reply to Martin Stránský from comment #5)
> It does not address the main issue when predefined stackSize is too low

The stack size is not supposed to match the native stack size. Rather it is supposed to detect run-away recursion and give a meaningful error reports long before the stack is exhausted.

If the predefined stacksize is too low, then we should increase it if it prevents some web site to run, but we should investigate why this happens. So what is the stack trace that lead to the assert violation?

Comment 7

[Martin Stránský [:stransky] (ni? me)]

(In reply to Igor Bukanov from comment #6)
> (In reply to Martin Stránský from comment #5)
> > It does not address the main issue when predefined stackSize is too low
> 
> The stack size is not supposed to match the native stack size. Rather it is
> supposed to detect run-away recursion and give a meaningful error reports
> long before the stack is exhausted.
> 
> If the predefined stacksize is too low, then we should increase it if it
> prevents some web site to run, but we should investigate why this happens.
> So what is the stack trace that lead to the assert violation?

It randomly fails right after start in js and/or firefox/thunderbird and it depends on recent stackDummy position. There are "too many recursion" warnings, ff fails to start and crashes in GC:

[/dist/bin]$ ./firefox -P default -no-remote
pldhash: for the table at address 0xb63188e8, the given entrySize of 48 probably favors chaining over double hashing.
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Cannot create startup observer : service,@mozilla.org/weave/service;1: file nsAppStartupNotifier.cpp, line 119
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Cannot create startup observer : service,@mozilla.org/embeddor.implemented/web-content-handler-registrar;1: file nsAppStartupNotifier.cpp, line 119
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Cannot create startup observer : service,@mozilla.org/browser/browserglue;1: file nsAppStartupNotifier.cpp, line 119
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Cannot create startup observer : service,@mozilla.org/privatebrowsing;1: file nsAppStartupNotifier.cpp, line 119
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Cannot create startup observer : service,@mozilla.org/browser/sessionstartup;1: file nsAppStartupNotifier.cpp, line 119
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Cannot create startup observer : service,@mozilla.org/appshell/trytoclose;1: file nsAppStartupNotifier.cpp, line 119
JS Component Loader: ERROR (null):0
                     too much recursion
WARNING: Failed to create Addons Manager.: file nsXREDirProvider.cpp, line 737
JS Component Loader: ERROR (null):0
                     too much recursion
JS Component Loader: ERROR (null):0
                     too much recursion
pldhash: for the table at address 0xb7c07068, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0xb7c07000 == 1
pldhash: for the table at address 0xb63f5cd0, the given entrySize of 48 probably favors chaining over double hashing.
WARNING: NS_ENSURE_TRUE(shell) failed: file nsDocShell.cpp, line 10805
WARNING: NS_ENSURE_TRUE(sf) failed: file nsDocShell.cpp, line 4970
WARNING: NS_ENSURE_TRUE(shell) failed: file nsDocShell.cpp, line 10805
WARNING: NS_ENSURE_TRUE(sf) failed: file nsDocShell.cpp, line 4970
++DOMWINDOW == 1 (0xb63bb378) [serial = 1] [outer = (nil)]
JS Component Loader: ERROR (null):0
                     too much recursion
JS Component Loader: ERROR (null):0
                     too much recursion
JS Component Loader: ERROR (null):0
                     too much recursion
--DOMWINDOW == 0 (0xb63bb378) [serial = 1] [outer = (nil)] [url = ]
--DOCSHELL 0xb7c07000 == 0
Assertion failure: JS_CHECK_STACK_SIZE(cx->stackLimit - (1 << 14), &stackDummy), at jsgc.cpp:2602

(gdb) bt
#0  0x003497a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0x003eebe6 in __nanosleep_nocancel () from /lib/tls/libc.so.6
#2  0x003ee9ec in sleep () from /lib/tls/libc.so.6
#3  0x00727859 in ah_crap_handler (signum=6) at nsSigHandlers.cpp:119
#4  0x0072bc7d in nsProfileLock::FatalSignalHandler (signo=6, info=0xbfe5c4d8, context=0xbfe5c558) at nsProfileLock.cpp:226
#5  <signal handler called>
#6  0x003497a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#7  0x004dc8f4 in raise () from /lib/tls/libpthread.so.0
#8  0x023219d3 in JS_Assert (s=0x281d8a0 "JS_CHECK_STACK_SIZE(cx->stackLimit - (1 << 14), &stackDummy)", 
    file=0x281c294 "jsgc.cpp", ln=2602) at jsutil.cpp:89
#9  0x02220956 in js_GC (cx=0xb7c27ec0, comp=0x0, gckind=GC_NORMAL) at jsgc.cpp:2602
#10 0x02185ca0 in JS_GC (cx=0xb7c27ec0) at jsapi.cpp:2665
#11 0x0072126c in nsXREDirProvider::DoShutdown (this=0xbfe5cc10) at nsXREDirProvider.cpp:804
#12 0x00711946 in ~ScopedXPCOMStartup (this=0xbfe5cc70) at nsAppRunner.cpp:1103
#13 0x00717c97 in XRE_main (argc=4, argv=0xbfe5cf34, aAppData=0xb7c0d380) at nsAppRunner.cpp:3784
#14 0x08049869 in main (argc=4, argv=0xbfe5cf34) at nsBrowserApp.cpp:158

It fails in js shell too:
[src]$ ./js
too much recursion
Assertion failure: JS_CHECK_STACK_SIZE(cx->stackLimit - (1 << 14), &stackDummy), at jsgc.cpp:2602
Aborted

(gdb) bt
#0  0x003497a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0x004dc8f4 in raise () from /lib/tls/libpthread.so.0
#2  0x0821973b in JS_Assert (s=0x82b0a38 "JS_CHECK_STACK_SIZE(cx->stackLimit - (1 << 14), &stackDummy)", 
    file=0x82af42c "jsgc.cpp", ln=2602) at jsutil.cpp:89
#3  0x0811778e in js_GC (cx=0x898b158, comp=0x0, gckind=GC_LAST_CONTEXT) at jsgc.cpp:2602
#4  0x080bf842 in js_DestroyContext (cx=0x898b158, mode=JSDCM_FORCE_GC) at jscntxt.cpp:1062
#5  0x08081239 in JS_DestroyContext (cx=0x898b158) at jsapi.cpp:1023
#6  0x0804f1c4 in DestroyContext (cx=0x898b158, withGC=true) at js.cpp:5696
#7  0x08059caf in main (argc=0, argv=0xbfea8bf8, envp=0xbfea8bfc) at js.cpp:5941

It may be caused by some strange stack configuration (it happens on RHEL4/gcc4 only so far) where the stackDummy is placed at the end of the stack.

btw. I'm on PTO next week.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: v3, enable/disable stack check

What about this one? It bounds stackSize by the native size by default and it allows to disable stack check on some broken platforms.

Comment 9

[Igor Bukanov]

(In reply to Martin Stránský from comment #8)
> Created attachment 553130 [details] [diff] [review]
> v3, enable/disable stack check

I think we should just disable that assert on Linux. Also I have missed that you mentioned in the comment 0 "because of small gMaxStackSize". But what is gMaxStackSize in your case?

Comment 10

[Martin Stránský [:stransky] (ni? me)]

(In reply to Igor Bukanov from comment #9)
> I think we should just disable that assert on Linux. Also I have missed that
> you mentioned in the comment 0 "because of small gMaxStackSize". But what is
> gMaxStackSize in your case?

gMaxStackSize is 50KB in optimized build, 500KB in debug build. I think the stack check works as expected, the problem is only on RHEL4 which is an ancient system.

Comment 11

[Igor Bukanov]

(In reply to Martin Stránský from comment #10)
> 
> gMaxStackSize is 50KB in optimized build, 500KB in debug build. I think the
> stack check works as expected, the problem is only on RHEL4 which is an
> ancient system.

Hm, but where this 50K comes from? If it is too low, why not just set it to a bigger value on affected systems at the point where JS_SetStackLimit  is called?

Comment 12

[Martin Stránský [:stransky] (ni? me)]

The system stack has 10MB (0xa00000), 50KB or 500KB makes no difference here. 

But I don't think it's a problem with mozilla stack size - it works fine on Fedora and RHEL6/5 with 50/500KB stack. The problem is a weird gcc on RHEL4 which keeps local variables (the stackDummy) on different stack frame (maybe?) than the return address. 

So the "big" value would be the whole stack size there, which is actually performed by the patch when stack check is disabled.

Comment 13

[Igor Bukanov]

(In reply to Martin Stránský from comment #12)
> So the "big" value would be the whole stack size there, which is actually
> performed by the patch when stack check is disabled.

So what about just disabling the stack size assert in js_GC() on that specific platform?

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Attachment: v4. disable the test macro

Do you mean something like this? It's missing the maximal stack size check, but I'm fine with it too.

Comment 15

[Igor Bukanov]

(In reply to Martin Stránský from comment #14)
> Do you mean something like this? It's missing the maximal stack size check,
> but I'm fine with it too.

Yes, this should be OK unless there is better workaround. For example, cannot we use some inline assembly or __builtin_frame_address(0) on that platform instead of &stackDummy?

Comment 16

[Martin Stránský [:stransky] (ni? me)]

__builtin_frame_address(0) suffers the same symptoms like &stackDummy. Fiddling with inline assembly may help but I'm not sure how reliable such code would be. I guess to disable it is the best for broken platforms for now.

Comment 17

[Andreas Gal :gal]

Disabling the stack check is not a good idea. Its easily exploitable for at least a DOS or worse.

Comment 18

[Igor Bukanov]

(In reply to Martin Stránský from comment #16)
> __builtin_frame_address(0) suffers the same symptoms like &stackDummy.

Hm, what would happen if you switch to -O2 optimization level and remove -fomit-frame-pointer from MODULE_OPTIMIZE_FLAGS in js/src/Makefile.in ?

Comment 19

[Martin Stránský [:stransky] (ni? me)]

It's built as a debug build, w/o optimalization & -fomit-frame-pointer:

-fvisibility=hidden -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -W
overloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -pedantic -Wno-long-long -g -O0 -fno-exceptions -fno-strict-aliasing -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g -DUSE_SYS

Comment 20

[Igor Bukanov]

(In reply to Martin Stránský from comment #19)
> It's built as a debug build, w/o optimalization & -fomit-frame-pointer:

Do you the problem in optimized builds? I.e. do you see unexpected too-much-recurssion errors there?

Comment 21

[Martin Stránský [:stransky] (ni? me)]

Yes, it's the same with optimized builds.

Comment 22

[Igor Bukanov]

What about using &cx, not &stackDummy, in that macro that asserts? Would the address still be bogus? Also could you add register printout at the point of crash?

Comment 23

[Martin Stránský [:stransky] (ni? me)]

I did some investigations on the affected platform and it looks like the stack base (0xc0000000) is incorrect (at least it does not refer to application stack top).

Even the first variable on the stack (ScopedLogging log; in main()) is located at 0xbfe60e8f which 1.7MB down. So the correct patch may compare first stack entry from main() with the recent one instead of the one from thread data.

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Attachment: v5, compare clocal variables

This one compares local variables instead of the stack&pthread data. Stack base is set when the cx context is created but it may be moved somewhere else. It's a bit invasive but works (at least on Fedora/RHEL4).

Comment 25

[Igor Bukanov]

(In reply to Martin Stránský from comment #24)
> This one compares local variables instead of the stack&pthread data. 

So the issue is that pthread_getattr_np returns bogus data. Is it possible then to fix that in glibc? Or, if that is not possible, perhaps it would be possible to add some very specific Linux calls there?

> Stack
> base is set when the cx context is created 

Without conservative scanner a workaround like that at that place or at the place that initializes ThreadData structure would be OK. But as our GC relies on been able to accurately determine the native stack boundaries, that is still wrong.

So as a workaround I suggest to patch nspr wrappers that creates the new threads and record the address of the local variable there in a thread-local slot. Then on the platform in question GetNativeStackBaseImpl can query that slot. If that would be null, then the code can assume that this is the main thread and hard-code some number perhaps.

Comment 26

[Martin Stránský [:stransky] (ni? me)]

yeah, it's bug in pthread_getattr_np() so it should be fixed here. After all we're not going to ship the new firefox on the affected platform. Thanks for the assistance here! Closing now, just open/ping if anyone is interesting in a fix/workaround for it.