[webkit-reviews] review granted: [Bug 181096] Apply poisoning to more pointers in JSC. : [Attachment 330712] proposed patch w/ 32-bit build fix.

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 181096: Apply poisoning to more pointers in JSC.
https://bugs.webkit.org/show_bug.cgi?id=181096

Attachment 330712: proposed patch w/ 32-bit build fix.

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




--- Comment #11 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 330712
  --> https://bugs.webkit.org/attachment.cgi?id=330712
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