Bug (StackBounds) – StackBounds checker fails on custom stack implementations (typically in a coroutine setting)

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Slava Akhmechet]

Much of the code in JavaScriptCore (specifically the parser), rely on the StackBounds class to check that the interpreter doesn't blow the stack in deeply recursive situations. StackBounds sets the m_origin and m_bound member variables in its initialize() member in order to define the stack limits and has custom implementations for different platforms. We're embedding JavaScriptCore into our database project, and the bounds checking breaks down for us for the following reason. Our project is heavily event-driven, and instead of starting a thread per client, we start a thread per CPU core, and multiplex multiple coroutines within each thread. Our coroutine implementation manually manages the stack pointer register, and allocates the stack space for each coroutine on the heap. This is pretty common in event-driven systems, and is typically achieved via getcontext/setcontext API on POSIX platforms, and the Fibers API on Windows platforms. In order for us to successfully integrate JavaScriptCore we need it to be able to set custom stack bounds, as opposed to using the default implementation provided by the StackBounds class.

I propose adding a function to the API that explicitly sets the stack bounds for a given JSContext. It would be immensely useful for projects that have custom stack implementations:

JS_EXPORT bool JSSetStackBounds(JSContextRef ctx, void *origin, void *bound);

This would set m_origin and m_bound members of the StackBounds class for the given context. I am working on the patch now and would love to see it committed to the tree, however I am new to WebKit and would appreciate any guidance or suggestions for making this change (or perhaps structuring it differently).

[Slava Akhmechet]

Created attachment 102421 [details]
Proposed patch

The patch is slightly different from the original proposal since I realized the bounds need to be set for context groups, not contexts.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp..." exit_code: 1

Source/JavaScriptCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 10 files


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

[Slava Akhmechet]

Created attachment 102423 [details]
Proposed patch v2 (fixed style errors in Changelog)

[Darin Adler]

Comment on attachment 102423 [details]
Proposed patch v2 (fixed style errors in Changelog)

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

I think that Geoff Garen or Oliver Hunt should take a look at this before we land it. Geoff will be away for a while because he is getting married today and is going on a honeymoon, so Oliver may be the best bet.

Mark Rowe may also be a good person to help us properly add new API getting the version checking right and having it exported properly.

> Source/JavaScriptCore/API/JSContextRef.cpp:70
> +    RefPtr<JSGlobalData> globalData = PassRefPtr<JSGlobalData>(toJS(group));
> +    globalData->stack()->setBounds(origin, bound);

There is no reason to put this into a local variable or use a RefPtr. It’s also not helpful and wrong style to use PassRefPtr here at all. It should just be toJS(group)->stack()->setBounds

> Source/JavaScriptCore/API/JSContextRef.h:80
> +JS_EXPORT void JSSetStackBounds(JSContextGroupRef, void *origin, void *bound) AVAILABLE_IN_WEBKIT_VERSION_4_0;

AVAILABLE_IN_WEBKIT_VERSION_4_0 is incorrect. This is something we just added, so it wasn’t available before.

In this file arguments need names, because it’s a C header file, not C++, so JSContextGroupRef needs the name "group" as you see above in other functions.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:588
> -        StackBounds m_stack;
> +        StackBounds *m_stack;

Formatting here is incorrect. The * goes next to the type.

> Source/JavaScriptCore/runtime/JSGlobalData.h:294
> -        StackBounds m_stack;
> +        StackBounds *m_stack;

Same problem with placement of the *.

> Source/JavaScriptCore/wtf/StackBounds.h:81
> +    void setBounds(void *origin, void *bound)

Incorrect formatting here. It's void* not void * in WebKit code.

[Slava Akhmechet]

(In reply to comment #4)
> (From update of attachment 102423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102423&action=review
Thanks for the feedback and sorry for the style problems. I relied on the check-webkit-style script, I didn't realize it doesn't catch issues like these.

I can fix these any time - I'd love to get Oliver's and Mark's feedback to see if there are other improvements I can make.

[Oliver Hunt]

One problem with this approach is that it assumes a single stack is used per context, which is not the case for JSC -- a single JSC context can be used on multiple threads (just not concurrently), so setting the stack bounds for a "context" doesn't necessarily make sense.

That aside, the new function should go into JSContextRefPrivate.h -- very few APIs ever get to go straight into a public header as we require API and ABI stability: once it's public we can't remove it.  Given this change is very specific to your use case I'm dubious as to its wider value.

It seems to me that a better solution would be for JSC to have a mechanism that checked the actual stack extent that it's executing on, rather than requiring developers that are using this technique to jump through hoops to get the correct behavior.

[Slava Akhmechet]

(In reply to comment #6)
> One problem with this approach is that it assumes a single stack is used per context, which is not the case for JSC -- a single JSC context can be used on multiple threads (just not concurrently), so setting the stack bounds for a "context" doesn't necessarily make sense.

Yes, the idea is that every time the host program calls into the interpreter from a different coroutine, it would have to make the JSSetStackBounds call first. The same would apply to different threads, except the host would have to ensure it does proper serialization. JSC context group seems like the most logical object to set the bounds on, I can't think of a better way.

> That aside, the new function should go into JSContextRefPrivate.h
Thanks, will do. The change isn't *too* specific to us - custom lightweight threading implementations are actually pretty common, but it's definitely not something most embedders do.

> It seems to me that a better solution would be for JSC to have a mechanism that checked the actual stack extent that it's executing on, rather than requiring developers that are using this technique to jump through hoops to get the correct behavior.

Unfortunately with custom stack implementations that's not possible (or at least I can't think of a way to do it). The stack could start and end at essentially any location. It's easy for JSC to check where it currently is on the stack, but impossible to know where it ends without the host telling it. Since the boundary can change with every call into the interpreter (due to a coroutine switch), JSC must be notified every time there is a coroutine switch. The only way to tell automatically is to set up a protocol where the host can put some magic value and then have JSC scan the stack for it (or, alternatively, have the host unmap a page just after the stack and have JSC catch the fault signal), but that still requires a protocol, in addition to signal trickery. I think that's much worse (and more complex) than a clean API call.

Thanks for all the feedback - I'll make the changes.

[Slava Akhmechet]

Created attachment 102539 [details]
Proposed patch v2 (fixed more style errors, moved export to private header)

Made all the relevant changes as discussed.

[Geoffrey Garen]

Comment on attachment 102539 [details]
Proposed patch v2 (fixed more style errors, moved export to private header)

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

> Source/JavaScriptCore/API/JSContextRef.cpp:67
> +void JSSetStackBounds(JSContextGroupRef group, void* origin, void* bound)

Our namespacing convention calls for "JSContextGroupSetStackBounds".

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:210
>          m_stack = wtfThreadData().stack();
> +    else
> +        m_stack = new StackBounds();

This doesn't seem right. For globalDataType != Default, m_stack gets set to "new StackBounds()", but stack() returns wtfThreadData().stack().

[Slava Akhmechet]

(In reply to comment #9)
> Our namespacing convention calls for "JSContextGroupSetStackBounds".
Sorry, will change.

> > Source/JavaScriptCore/runtime/JSGlobalData.cpp:210
> >          m_stack = wtfThreadData().stack();
> > +    else
> > +        m_stack = new StackBounds();
> 
> This doesn't seem right. For globalDataType != Default, m_stack gets set to "new StackBounds()", but stack() returns wtfThreadData().stack().

I agree, but this is equivalent to the code that's in place right now. Currently JSGlobalData constructs m_stack via the default constructor, and later conditionally overwrites it as follows:

207    if (globalDataType == Default)
208        m_stack = wtfThreadData().stack();

The member function stack() is defined as follows:

211        const StackBounds& stack()
212        {
213            return (globalDataType == Default)
214                ? m_stack
215                : wtfThreadData().stack();
216        }

I didn't want to dive into this and make changes because they're outside the scope of the patch. Do you have thoughts on this?

[Slava Akhmechet]

Hey guys, does anyone have feedback on this?

(In reply to comment #10)
> (In reply to comment #9)
> > Our namespacing convention calls for "JSContextGroupSetStackBounds".
> Sorry, will change.
> 
> > > Source/JavaScriptCore/runtime/JSGlobalData.cpp:210
> > >          m_stack = wtfThreadData().stack();
> > > +    else
> > > +        m_stack = new StackBounds();
> > 
> > This doesn't seem right. For globalDataType != Default, m_stack gets set to "new StackBounds()", but stack() returns wtfThreadData().stack().
> 
> I agree, but this is equivalent to the code that's in place right now. Currently JSGlobalData constructs m_stack via the default constructor, and later conditionally overwrites it as follows:
> 
> 207    if (globalDataType == Default)
> 208        m_stack = wtfThreadData().stack();
> 
> The member function stack() is defined as follows:
> 
> 211        const StackBounds& stack()
> 212        {
> 213            return (globalDataType == Default)
> 214                ? m_stack
> 215                : wtfThreadData().stack();
> 216        }
> 
> I didn't want to dive into this and make changes because they're outside the scope of the patch. Do you have thoughts on this?

[Slava Akhmechet]

Could someone take a look at this? Just pinging to make sure it didn't slip through the cracks.

[Oliver Hunt]

I just realised that this behaviour may not be GC safe -- JSC is a conservative collector: at gc time we perform a conservative sweep of all machine stacks for references to GC objects.  By hoisting execution of the OS stack it seems that you would put JSC into a state where at least one set of roots can be missed.

How do you avoid this problem?

[Slava Akhmechet]

(In reply to comment #13)
> I just realised that this behaviour may not be GC safe -- JSC is a conservative collector: at gc time we perform a conservative sweep of all machine stacks for references to GC objects.  By hoisting execution of the OS stack it seems that you would put JSC into a state where at least one set of roots can be missed.
> 
> How do you avoid this problem?
Ahh, you're correct.  We get around this by writing our code and creating contexts and context groups in a very precise way. We could fix this, but it's getting increasingly hacky. We'll just work around this in our code, please feel free to close this, and sorry for using up the time.

[Gavin Barraclough]

Based on comments in this bug, looks like this isn't going ahead, so I'm going to close it.  This is meant with no prejudice! - if this becomes relevant again, please reopen.

Thanks, G.