Bug 181096 – Apply poisoning to more pointers in JSC.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/36182970>

[Mark Lam]

Created attachment 330132 [details]
work in progress.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 330617 [details]
work in progress for EWS testing.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 330662 [details]
x86_64 benchmark results.

Perf appears to be neutral.

[Mark Lam]

Created attachment 330665 [details]
proposed patch.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 330712 [details]
proposed patch w/ 32-bit build fix.

[EWS Watchlist]

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.

[JF Bastien]

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

[Mark Lam]

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.

[JF Bastien]

(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!

[Mark Lam]

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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 330720 [details]
patch for landing.

[EWS Watchlist]

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.

[Saam Barati]

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.

[Mark Lam]

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