Bug 148976 – GC stack scan should include ABI red zone

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The x86_64 ABI section 3.2.2 (http://people.freebsd.org/~obrien/amd64-elf-abi.pdf) and ARM64 ABI (https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW7) both states that there is a 128 byte red zone below the stack pointer (reserved by the OS), and that "functions may use this area for temporary data that is not needed across function calls".

Hence, it is possible for a thread to store JSCell pointers in the red zone area, and the conservative GC thread scanner needs to scan that area as well.

[Mark Lam]

For the record, this issue was reported by Andreas Kling.

[Mark Lam]

Created attachment 260790 [details]
the fix.

[Mark Lam]

Comment on attachment 260790 [details]
the fix.

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

> Source/JavaScriptCore/ChangeLog:14
> +        Hence, it is possible for a thread to store JSCell pointers in the red zone
> +        area, and the conservative GC thread scanner needs to scan that area as well.

Note: the red zone should not be scanned for the GC thread (in gatherFromCurrentThread()).  This because the we're guaranteed that there will be GC frames on top of the top most frame that we need to scan.  Hence, we are guaranteed that there are no red zone areas there containing JSObject pointers of relevance.

Hmm, maybe I should add this detail into the ChangeLog as well.

[Geoffrey Garen]

Comment on attachment 260790 [details]
the fix.

Is this adjustment valid even if execution has extended the stack to its limit? In other words, does the OS reported stack limit include or exclude the red zone?

[Mark Lam]

Created attachment 260803 [details]
fix 2

To address Geoff's question, I wasn't able to find any details of whether the OS will reserve some amount of stack memory for the red zone size (and the interrupt stack frames that it pushes on top of it). 

A search of the OSX pthread library shows that pthread_get_stacksize_np() actually returns the size value used to allocate the stack of the thread.  There's no padding for a red zone region.  This means that pthread makes no attempt to hold its user back from using the OS red zone region.

Based on this, I don't think it is safe to assume that the OS will some how reserve the red zone size (it may or may not).  To be cautious, I added a check to ensure that we do not scan beyond the end of the stack.

[Geoffrey Garen]

Comment on attachment 260803 [details]
fix 2

r=me

[Benjamin Poulain]

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

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:199
> -        return new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin());
> +        return new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin(), wtfThreadData().stack().end());

wtfThreadData() is not cheap IIRC. You should put the const StackBounds& in a temporary.

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:533
> +    char* begin = reinterpret_cast<char*>(stackBase);
> +    char* end = reinterpret_cast<char*>(

Shouldn't this be a reinterpret_cast_ptr()?

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:534
>          WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(stackTop)));

Could be on the previous line.

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:537
> +    ASSERT(begin >= end);
> +
> +    char* endWithRedZone = end + osRedZoneAdjustment();

I don't get this. The red zone is from SP, but here you are applying it from the rounded up address.

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:541
> +    if (endWithRedZone < stackEnd)
> +        endWithRedZone = reinterpret_cast<char*>(stackEnd);

I am quite confused by this, how could the red-zone extend past the stack end? The kernel does not enforce this?

[Mark Lam]

I've spoken with Ben offline, but will document our discussion below.

(In reply to comment #7)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260803&action=review
> 
> > Source/JavaScriptCore/heap/MachineStackMarker.cpp:199
> > -        return new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin());
> > +        return new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin(), wtfThreadData().stack().end());
> 
> wtfThreadData() is not cheap IIRC. You should put the const StackBounds& in
> a temporary.

Done.  Fixed locally.

> > Source/JavaScriptCore/heap/MachineStackMarker.cpp:533
> > +    char* begin = reinterpret_cast<char*>(stackBase);
> > +    char* end = reinterpret_cast<char*>(
> 
> Shouldn't this be a reinterpret_cast_ptr()?

Done.  Fixed locally.

> > Source/JavaScriptCore/heap/MachineStackMarker.cpp:534
> >          WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(stackTop)));
> 
> Could be on the previous line.

Done.  Fixed locally.

> > Source/JavaScriptCore/heap/MachineStackMarker.cpp:537
> > +    ASSERT(begin >= end);
> > +
> > +    char* endWithRedZone = end + osRedZoneAdjustment();
> 
> I don't get this. The red zone is from SP, but here you are applying it from
> the rounded up address.

It is assumed that the osRedZoneAdjustment() value will be properly aligned.  Hence, adding it to end should not break the alignment.  This is asserted right after the computation of endWithRedZone.

If there are stray bytes between end and stackTop, the same number of stray bytes will appear between endWithRedZone and stackTop + osRedZoneAdjustment().  Since pointers are always stored on an aligned address, it is safe to assume that those stray bytes will not contain a pointer.

So, it is safe to compute endWithRedZone this way.

> > Source/JavaScriptCore/heap/MachineStackMarker.cpp:541
> > +    if (endWithRedZone < stackEnd)
> > +        endWithRedZone = reinterpret_cast<char*>(stackEnd);
> 
> I am quite confused by this, how could the red-zone extend past the stack
> end? The kernel does not enforce this?

The scenario that I was thinking of is one where the function in the top most frame (bottom of the stack) does not use the red zone for locals.  Hence, its topOfStack pointer can go near the end of the stack but does not necessarily leave enough room for the red zone size.  This is correct behavior for that function.

Our conservative scanner, on the other hand, does not know that the top function does not use the red zone.  If we blindly add the red zone size without checking the stack bounds, then the GC thread could theoretically scan pass the end of the stack (and crash).  The bounds check ensures that this does not happen.  This is the conservative thing to do.

[Mark Lam]

Thanks for the reviews.  Landed in r189517: <http://trac.webkit.org/r189517>.

[Michael Saboff]

Comment on attachment 260803 [details]
fix 2

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

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:526
> +#if !OS(WINDOWS)
> +#if CPU(X86_64)
> +    // See http://people.freebsd.org/~obrien/amd64-elf-abi.pdf Section 3.2.2.
> +    redZoneAdjustment = -128;
> +#elif CPU(ARM64)
> +    // See https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW7
> +    redZoneAdjustment = -128;
> +#endif
> +#endif // OS(DARWIN)

Is this for all non Windows platforms or Darwin only?  Use a consistent OS type for the #if OS() and the #endif comment.

[Mark Lam]

(In reply to comment #10)
> > +#endif // OS(DARWIN)
> 
> Is this for all non Windows platforms or Darwin only?  Use a consistent OS
> type for the #if OS() and the #endif comment.

Eeek.  Comment update fail.  Thanks for catching that.

Fixed in r189520: <http://trac.webkit.org/r189520>.