Bug 181096 - Apply poisoning to more pointers in JSC.
Summary: Apply poisoning to more pointers in JSC.
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: 2017-12-21 12:03 PST by Mark Lam
Modified: 2018-01-08 13:05 PST (History)
10 users (show)

See Also:


Attachments
work in progress. (43.88 KB, patch)
2017-12-22 11:12 PST, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (69.95 KB, patch)
2018-01-05 17:20 PST, Mark Lam
no flags Details | Formatted Diff | Diff
x86_64 benchmark results. (8.09 KB, text/plain)
2018-01-07 15:31 PST, Mark Lam
no flags Details
proposed patch. (76.85 KB, patch)
2018-01-07 18:06 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch w/ 32-bit build fix. (76.87 KB, patch)
2018-01-08 10:12 PST, Mark Lam
jfbastien: review+
Details | Formatted Diff | Diff
patch for landing. (77.49 KB, patch)
2018-01-08 12:10 PST, 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 2017-12-21 12:03:31 PST
Patch coming.
Comment 1 Radar WebKit Bug Importer 2017-12-21 12:06:49 PST
<rdar://problem/36182970>
Comment 2 Mark Lam 2017-12-22 11:12:08 PST
Created attachment 330132 [details]
work in progress.
Comment 3 EWS Watchlist 2017-12-22 11:15:07 PST
Attachment 330132 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:111:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:92:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:392:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:393:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1318:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1318:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1319:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 9 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mark Lam 2018-01-05 17:20:01 PST
Created attachment 330617 [details]
work in progress for EWS testing.
Comment 5 EWS Watchlist 2018-01-05 17:21:10 PST
Attachment 330617 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Bag.h:50:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:392:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:393:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1323:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1323:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1324:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:111:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:92:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 9 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Lam 2018-01-07 15:31:07 PST
Created attachment 330662 [details]
x86_64 benchmark results.

Perf appears to be neutral.
Comment 7 Mark Lam 2018-01-07 18:06:43 PST
Created attachment 330665 [details]
proposed patch.
Comment 8 EWS Watchlist 2018-01-07 18:08:27 PST
Attachment 330665 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:402:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:403:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Mark Lam 2018-01-08 10:12:48 PST
Created attachment 330712 [details]
proposed patch w/ 32-bit build fix.
Comment 10 EWS Watchlist 2018-01-08 10:14:55 PST
Attachment 330712 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:402:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:403:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 JF Bastien 2018-01-08 10:42:11 PST
Comment on attachment 330712 [details]
proposed patch w/ 32-bit build fix.

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

r=me with some comments.

> Source/JavaScriptCore/assembler/MacroAssembler.h:1002
> +    }

This always uses an extra scratch. It's convenient to type less, but is it something we usually do (as opposed to being explicit in code that it'll necessarily need a scratch)? At least in some WebAssembly cases we don't opt-in to that scratch being usable, so it would assert to use this xorPtr.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:110
> +}

This seems like the wrong place for the helper? It should be in the poisoned unique ptr header.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1961
> +    else

This should be `else if X86_64_WIN`

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1037
>      loadp CodeBlock::m_vm[t1], t2

I think it would be nice to rename CodeBlock::m_vm to CodeBlock::m_PoisonedVm because places like this would be way more obvious. It's easy to miss an unpoison otherwise IMO.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:197
> +}

Ditto, wrong place.

> Source/WTF/wtf/Bag.h:47
> +    BagNode* m_next { nullptr };

So I understand correctly: we're allowing poison of head, and not next, because the head is the only thing "dangerously" accessible and you need it to get to any next node? In other words, we'd poison any BagNode* that's held elsewhere?

> Source/WTF/wtf/Bag.h:155
> +using PoisonedBag = Bag<T, ConstExprPoisonedPtrTraits<key, Private::BagNode<T>>>;

I gotta say PoisonedBag sounds badass. Now we need BagFullOfPoisonSnakes.

> Source/WTF/wtf/Poisoned.h:90
> +// FiXME: once we have C++17, we can make the key of type auto and remove the need for KeyType.

ALL CAPS FAIL ON FiXME
Comment 12 Mark Lam 2018-01-08 10:54:45 PST
Comment on attachment 330712 [details]
proposed patch w/ 32-bit build fix.

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

Thanks for the review.

>> Source/JavaScriptCore/assembler/MacroAssembler.h:1002
>> +    }
> 
> This always uses an extra scratch. It's convenient to type less, but is it something we usually do (as opposed to being explicit in code that it'll necessarily need a scratch)? At least in some WebAssembly cases we don't opt-in to that scratch being usable, so it would assert to use this xorPtr.

I'll see what I can do about this to make it give the option to pass in a scratch.  May leave as is for now.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:110
>> +}
> 
> This seems like the wrong place for the helper? It should be in the poisoned unique ptr header.

I disagree.  This is a special version of makePoisonedUnique (using CodeBlockPoison) only for use in this CodeBlock.cpp file.  Hence, this is the right place for it.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1961
>> +    else
> 
> This should be `else if X86_64_WIN`

I disagree.  The else case is generic for all !POISON cases.  This is why I changed the condition being tested here.  My goal is to minimize the burden on builds that don't support poisoning.  Hence, I believe this code is correct as is.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1037
>>      loadp CodeBlock::m_vm[t1], t2
> 
> I think it would be nice to rename CodeBlock::m_vm to CodeBlock::m_PoisonedVm because places like this would be way more obvious. It's easy to miss an unpoison otherwise IMO.

I'll look into this.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:197
>> +}
> 
> Ditto, wrong place.

Again, I disagree.  This is a convenience version of makePoisonedUnique that is only used in JSGlobalObject.cpp, and uses only JSGlobalObjectPoison.  Hence, it belongs here.

>> Source/WTF/wtf/Bag.h:47
>> +    BagNode* m_next { nullptr };
> 
> So I understand correctly: we're allowing poison of head, and not next, because the head is the only thing "dangerously" accessible and you need it to get to any next node? In other words, we'd poison any BagNode* that's held elsewhere?

BagNode is only held in Bag (hence, my putting it in a WTF::Private namespace).  And yes, we only need to poison the head in Bag since Bag can be embedded in client data structures.

>> Source/WTF/wtf/Poisoned.h:90
>> +// FiXME: once we have C++17, we can make the key of type auto and remove the need for KeyType.
> 
> ALL CAPS FAIL ON FiXME

Will fix.
Comment 13 JF Bastien 2018-01-08 11:07:48 PST
(In reply to Mark Lam from comment #12)
> Comment on attachment 330712 [details]
> proposed patch w/ 32-bit build fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330712&action=review
> 
> Thanks for the review.
> 
> >> Source/JavaScriptCore/assembler/MacroAssembler.h:1002
> >> +    }
> > 
> > This always uses an extra scratch. It's convenient to type less, but is it something we usually do (as opposed to being explicit in code that it'll necessarily need a scratch)? At least in some WebAssembly cases we don't opt-in to that scratch being usable, so it would assert to use this xorPtr.
> 
> I'll see what I can do about this to make it give the option to pass in a
> scratch.  May leave as is for now.

Yeah checking this in for now is fine. I'd put a FIXME with bug though.

> >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:110
> >> +}
> > 
> > This seems like the wrong place for the helper? It should be in the poisoned unique ptr header.
> 
> I disagree.  This is a special version of makePoisonedUnique (using
> CodeBlockPoison) only for use in this CodeBlock.cpp file.  Hence, this is
> the right place for it.

What I mean is that you can have a generic version to which you pass in the special bits, no?

> >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1961
> >> +    else
> > 
> > This should be `else if X86_64_WIN`
> 
> I disagree.  The else case is generic for all !POISON cases.  This is why I
> changed the condition being tested here.  My goal is to minimize the burden
> on builds that don't support poisoning.  Hence, I believe this code is
> correct as is.

Oh I guess I mis-read the diff!
Comment 14 Mark Lam 2018-01-08 12:01:29 PST
Comment on attachment 330712 [details]
proposed patch w/ 32-bit build fix.

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

>>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:110
>>>> +}
>>> 
>>> This seems like the wrong place for the helper? It should be in the poisoned unique ptr header.
>> 
>> I disagree.  This is a special version of makePoisonedUnique (using CodeBlockPoison) only for use in this CodeBlock.cpp file.  Hence, this is the right place for it.
> 
> What I mean is that you can have a generic version to which you pass in the special bits, no?

I think the concern here is that these file specific versions may cause conflicts in our unified files build system.  I'll make these into static private methods of the class.
Comment 15 Mark Lam 2018-01-08 12:06:45 PST
Comment on attachment 330712 [details]
proposed patch w/ 32-bit build fix.

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

>>>> Source/JavaScriptCore/assembler/MacroAssembler.h:1002
>>>> +    }
>>> 
>>> This always uses an extra scratch. It's convenient to type less, but is it something we usually do (as opposed to being explicit in code that it'll necessarily need a scratch)? At least in some WebAssembly cases we don't opt-in to that scratch being usable, so it would assert to use this xorPtr.
>> 
>> I'll see what I can do about this to make it give the option to pass in a scratch.  May leave as is for now.
> 
> Yeah checking this in for now is fine. I'd put a FIXME with bug though.

I'll do this in a follow up.

>>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1037
>>>      loadp CodeBlock::m_vm[t1], t2
>> 
>> I think it would be nice to rename CodeBlock::m_vm to CodeBlock::m_PoisonedVm because places like this would be way more obvious. It's easy to miss an unpoison otherwise IMO.
> 
> I'll look into this.

I'll do this renaming in a follow up patch.
Comment 16 Mark Lam 2018-01-08 12:10:27 PST
Created attachment 330720 [details]
patch for landing.
Comment 17 EWS Watchlist 2018-01-08 12:12:13 PST
Attachment 330720 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:392:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:393:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:191:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Saam Barati 2018-01-08 12:22:46 PST
Comment on attachment 330720 [details]
patch for landing.

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

> Source/JavaScriptCore/bytecode/CodeBlock.h:115
> +        CodeBlock& codeBlock;

style nit: We usually use pointers instead of references for heap cells.
Comment 19 Mark Lam 2018-01-08 13:05:58 PST
Landed in r226530: <http://trac.webkit.org/r226530>.