Crashes on PPC64 due to mprotect() on address not aligned to the page size

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michel Normand]

refer to details of the crash in referenced fedora bugzilla

[Michel Normand]

Created attachment 226710 [details]
webkitgtk3.commitsize.to.pagesize.patch

a first RFC patch to avoid the Crash.

[Dmitry Shachnev]

We are getting a similar crash in Ubuntu. Has there been any progress on reviewing the patch?

[Alberto Garcia]

Created attachment 241283 [details]
Patch

Thanks for the patch. I don't know the JSC internals so I cannot review it myself.

However defining a local 'commitsize' to override the global constant 'commitSize' is very confusing. Either use a different name or use pageSize() directly.

[Alberto Garcia]

Created attachment 266795 [details]
Make commitSize at least as big as the page size

Here's a new patch. It makes sure that commitSize is at least as big as the page size without decreasing it if it's less than 16 KB.

[Mark Lam]

Comment on attachment 266795 [details]
Make commitSize at least as big as the page size

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

commitSIze is only needed when "#if !ENABLE(JIT)".  Let's put it in the appropriate sections.

> Source/JavaScriptCore/interpreter/JSStack.cpp:46
>  static StaticLock stackStatisticsMutex;
>  #endif // !ENABLE(JIT)
>  
> +static size_t commitSize;

Move the commitSize declaration just below committedBytesCount above.

> Source/JavaScriptCore/interpreter/JSStack.cpp:58
> +    commitSize = std::max(16 * 1024, getpagesize());
> +
>  #if !ENABLE(JIT)

Move this initialization below the #if !ENABLE(JIT).

Also, it may not matter much but the commitSize value should only be set once, not every time we construct a new JSStack.  Perhaps it would be better to have static function and use that instead wherever you use commitSize currently in JSStack.cpp:

static size_t commitSize()
{
    static size_t size = 0;
    if (!size)
        size = std::max(16 * 1024, getpagesize());
    return size;
}

[Alberto Garcia]

Created attachment 266803 [details]
Make commitSize at least as big as the page size (v2)

Thanks for your comments, here's the updated version of the patch.

[Mark Lam]

Comment on attachment 266803 [details]
Make commitSize at least as big as the page size (v2)

r=me

[Alberto Garcia]

Committed r193648: <http://trac.webkit.org/changeset/193648>

[Darin Adler]

Comment on attachment 266803 [details]
Make commitSize at least as big as the page size (v2)

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

> Source/JavaScriptCore/interpreter/JSStack.cpp:48
> +    static size_t size = 0;
> +    if (!size)
> +        size = std::max(16 * 1024, getpagesize());
> +    return size;

The check for 0 here is unneeded and unhelpful. This should just be:

    static size_t size = std::max<size_t>(16 * 1024, pageSize());
    return size;

[Alberto Garcia]

(In reply to comment #9)
> > Source/JavaScriptCore/interpreter/JSStack.cpp:48
> > +    static size_t size = 0;
> > +    if (!size)
> > +        size = std::max(16 * 1024, getpagesize());
> > +    return size;
>
> The check for 0 here is unneeded and unhelpful. This should just be:
>
>     static size_t size = std::max<size_t>(16 * 1024, pageSize());
>     return size;

You're right, I'll commit that change. Thanks!