[webkit-reviews] review granted: [Bug 182155] Apply poisoning to TypedArray vector pointers. : [Attachment 332680] proposed patch.

[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 182155: Apply poisoning to TypedArray vector pointers.
https://bugs.webkit.org/show_bug.cgi?id=182155

Attachment 332680: proposed patch.

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




--- Comment #42 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 332680
  --> https://bugs.webkit.org/attachment.cgi?id=332680
proposed patch.

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6388
> +    GPRReg indexGPR = index.gpr();

Assert that poison/index aren't the same as base/vector?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6408
> +    m_jit.xorPtr(poisonGPR, vectorGPR);

Blow away the poisonGPR content after?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3536
> +	   poisonedVector = m_out.bitXor(poisonedVector, poison);

Zero out the poison Lvalue after?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:402
> +	   xorp scratch, dest

No need to zero out scratch if GIGACAGE_ENABLED and not C_LOOP because it'll be
clobbered. I guess we don't care otherwise?

> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:34
> +template<typename Poison, typename T> struct PoisonedPtrTraits;

Include Forwards.h instead.

> Source/JavaScriptCore/runtime/JSArrayBufferView.h:226
> +    PoisonedCagedBarrierPtr<Poison, Gigacage::Primitive, void>
m_poisonedVector;

🤯 a vector of poisons, held in a poisoned caged barrier pointer 🤯

> Source/WTF/wtf/CagedPtr.h:38
> +    explicit CagedPtr(T* ptr = nullptr)

Do you want to add a nullptr_t ctor since this one is now explicit (if someone
doesn't want to rely on default)?

> Source/WTF/wtf/CagedPtr.h:98
> +    explicit CagedPtr(void* ptr = nullptr)

Ditto.