Bug 190405 - Changes towards allowing use of the ASAN detect_stack_use_after_return option.
Summary: Changes towards allowing use of the ASAN detect_stack_use_after_return option.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-09 10:57 PDT by Mark Lam
Modified: 2018-10-11 12:20 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch. (17.66 KB, patch)
2018-10-10 22:37 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (17.64 KB, patch)
2018-10-10 22:43 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (21.55 KB, patch)
2018-10-10 22:53 PDT, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff
patch for landing. (22.63 KB, patch)
2018-10-11 11:01 PDT, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff
patch for landing. (22.57 KB, patch)
2018-10-11 11:12 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-10-09 10:57:00 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-10-09 10:57:34 PDT
<rdar://problem/45131464>
Comment 2 Mark Lam 2018-10-10 22:37:54 PDT
Created attachment 352020 [details]
proposed patch.

Let's try this on the EWS bots.
Comment 3 Mark Lam 2018-10-10 22:43:12 PDT
Created attachment 352021 [details]
proposed patch.
Comment 4 EWS Watchlist 2018-10-10 22:47:57 PDT
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.
Comment 5 Mark Lam 2018-10-10 22:53:17 PDT
Created attachment 352022 [details]
proposed patch.
Comment 6 Michael Saboff 2018-10-11 09:22:12 PDT
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?
Comment 7 Mark Lam 2018-10-11 09:44:14 PDT
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.
Comment 8 Mark Lam 2018-10-11 11:01:57 PDT
Created attachment 352056 [details]
patch for landing.
Comment 9 EWS Watchlist 2018-10-11 11:03:57 PDT
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.
Comment 10 Michael Saboff 2018-10-11 11:10:47 PDT
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.
Comment 11 Mark Lam 2018-10-11 11:12:14 PDT
Created attachment 352057 [details]
patch for landing.
Comment 12 EWS Watchlist 2018-10-11 11:13:56 PDT
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.
Comment 13 Mark Lam 2018-10-11 12:20:06 PDT
Landed in r237042: <http://trac.webkit.org/r237042>.