Bug 130237 - Crashes on PPC64 due to mprotect() on address not aligned to the page size
Summary: Crashes on PPC64 due to mprotect() on address not aligned to the page size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL: https://bugzilla.redhat.com/show_bug....
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-14 07:11 PDT by Michel Normand
Modified: 2015-12-08 08:29 PST (History)
10 users (show)

See Also:


Attachments
webkitgtk3.commitsize.to.pagesize.patch (2.40 KB, patch)
2014-03-14 07:19 PDT, Michel Normand
no flags Details | Formatted Diff | Diff
Patch (2.09 KB, patch)
2014-11-10 01:41 PST, Alberto Garcia
no flags Details | Formatted Diff | Diff
Make commitSize at least as big as the page size (1.92 KB, patch)
2015-12-07 12:37 PST, Alberto Garcia
mark.lam: review-
Details | Formatted Diff | Diff
Make commitSize at least as big as the page size (v2) (2.79 KB, patch)
2015-12-07 13:30 PST, Alberto Garcia
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michel Normand 2014-03-14 07:11:35 PDT
refer to details of the crash in referenced fedora bugzilla
Comment 1 Michel Normand 2014-03-14 07:19:03 PDT
Created attachment 226710 [details]
webkitgtk3.commitsize.to.pagesize.patch

a first RFC patch to avoid the Crash.
Comment 2 Dmitry Shachnev 2014-11-06 05:45:58 PST
We are getting a similar crash in Ubuntu. Has there been any progress on reviewing the patch?
Comment 3 Alberto Garcia 2014-11-10 01:41:25 PST
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.
Comment 4 Alberto Garcia 2015-12-07 12:37:08 PST
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.
Comment 5 Mark Lam 2015-12-07 12:58:13 PST
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;
}
Comment 6 Alberto Garcia 2015-12-07 13:30:43 PST
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.
Comment 7 Mark Lam 2015-12-07 13:32:11 PST
Comment on attachment 266803 [details]
Make commitSize at least as big as the page size (v2)

r=me
Comment 8 Alberto Garcia 2015-12-07 13:35:29 PST
Committed r193648: <http://trac.webkit.org/changeset/193648>
Comment 9 Darin Adler 2015-12-08 08:03:01 PST
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;
Comment 10 Alberto Garcia 2015-12-08 08:29:30 PST
(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!