[webkit-reviews] review granted: [Bug 181339] WebAssembly: poison JS object's secrets : [Attachment 330598] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 181339: WebAssembly: poison JS object's secrets
https://bugs.webkit.org/show_bug.cgi?id=181339

Attachment 330598: patch

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




--- Comment #14 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 330598
  --> https://bugs.webkit.org/attachment.cgi?id=330598
patch

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

r=me with fixes.

> Source/JavaScriptCore/runtime/JSCPoison.h:40
>      TransitionMapPoison,
>      WeakImplPoison,
> +    JSWebAssemblyCodeBlockPoison,
> +    JSWebAssemblyInstancePoison,
> +    JSWebAssemblyMemoryPoison,
> +    JSWebAssemblyModulePoison,
> +    JSWebAssemblyTablePoison,

Order doesn't really matter, but let's keep these alphabetically ordered (with
NotPoisoned being the only exception) to help us read the code.  When the list
gets large, this will help.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:121
> +	      
jit.move(CCallHelpers::TrustedImm64(WTF::makeConstExprPoison(JSWebAssemblyInsta
ncePoison)), scratchReg);

I meant to fix this but I haven't done so yet: please remove the WTF::
qualifier here and add "using WTF::makeConstExprPoison" in Poisoned.h.

> Source/WTF/wtf/Poisoned.h:190
> +    void swap(T& t2)
> +    {
> +	   T t1 = this->unpoisoned();
> +	   std::swap(t1, t2);
> +	   m_poisonedBits = poison(t1);
> +    }

Please add test cases to TestWebKitAPI/.../Poisoned.cpp and PoisonedRef.cpp