Bug 190405 – Changes towards allowing use of the ASAN detect_stack_use_after_return option.

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

[Mark Lam]

The ASAN detect_stack_use_after_return options makes a assumptions about how code should behave that does not jive well with how a VM needs to operate.  In order to enable the use of detect_stack_use_after_return, we'll need to suppress ASAN in any function that takes the address of a variable in the current stack frame.  

We have not yet isolated all the places where we need to apply workarounds needed for ASAN detect_stack_use_after_return, but this patch for landing what we have found so far.

[Radar WebKit Bug Importer]

<rdar://problem/45131464>

[Mark Lam]

Created attachment 352020 [details]
proposed patch.

Let's try this on the EWS bots.

[Mark Lam]

Created attachment 352021 [details]
proposed patch.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/ChangeLog:9:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use after free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 14 files


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

[Mark Lam]

Created attachment 352022 [details]
proposed patch.

[Michael Saboff]

Comment on attachment 352022 [details]
proposed patch.

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

r=me

> Source/WTF/wtf/StackPointer.cpp:39
> +    // Make sure that sp is the only local variable decalred in this function.

*declared*

> Source/WTF/wtf/StackPointer.cpp:40
> +    void* sp = reinterpret_cast<uint8_t*>(&sp) + sizeOfFrameHeader + sizeof(sp);

uint8_t should probably be a uintptr_t.

> Source/WTF/wtf/StackPointer.h:30
> +WTF_EXPORT_PRIVATE void* currentStackPointer();

Did you consider using inline assembly for architectures we know well?

[Mark Lam]

Comment on attachment 352022 [details]
proposed patch.

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

Thanks for the review.

>> Source/WTF/wtf/StackPointer.cpp:39
>> +    // Make sure that sp is the only local variable decalred in this function.
> 
> *declared*

Fixed.

>> Source/WTF/wtf/StackPointer.cpp:40
>> +    void* sp = reinterpret_cast<uint8_t*>(&sp) + sizeOfFrameHeader + sizeof(sp);
> 
> uint8_t should probably be a uintptr_t.

Actually this needs to be uint8_t* because I'm adding sizes in bytes to it.  I've verified in lldb that this computes the correct value.

>> Source/WTF/wtf/StackPointer.h:30
>> +WTF_EXPORT_PRIVATE void* currentStackPointer();
> 
> Did you consider using inline assembly for architectures we know well?

I did, but thought that it unnecessarily complicates the implementation.  I ended up removing the inline asm solution in favor of more simplicity since I didn't think that this will was used in the perf critical path.  On second thought, I'll re-install the inline asm implementation so that this function is more versatile and can be used without worrying about perf.

[Mark Lam]

Created attachment 352056 [details]
patch for landing.

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/StackPointer.h:39:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:39:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WTF/wtf/StackPointer.h:41:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:41:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WTF/wtf/StackPointer.h:44:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:44:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WTF/wtf/StackPointer.h:47:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:47:  Extra space before )  [whitespace/parens] [2]
Total errors found: 12 in 16 files


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

[Michael Saboff]

Comment on attachment 352056 [details]
patch for landing.

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

r=me

> Source/WTF/wtf/StackPointer.h:56
> +#endif
> +
> +#if USE(GENERIC_CURRENT_STACK_POINTER)

You can eliminate these lines.

[Mark Lam]

Created attachment 352057 [details]
patch for landing.

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/StackPointer.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/StackPointer.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 16 files


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

[Mark Lam]

Landed in r237042: <http://trac.webkit.org/r237042>.