[webkit-reviews] review granted: [Bug 182086] DirectArguments should protect itself using dynamic poisoning and precise index masking : [Attachment 332319] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 182086: DirectArguments should protect itself using dynamic poisoning and
precise index masking
https://bugs.webkit.org/show_bug.cgi?id=182086

Attachment 332319: the patch

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




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 332319
  --> https://bugs.webkit.org/attachment.cgi?id=332319
the patch

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

r=me w/ some naming nits and other tidbits

> Source/JavaScriptCore/runtime/DirectArguments.h:122
> +	   return *preciseIndexMaskPtr(offset.offset(), std::max(m_length,
m_minCapacity), dynamicPoison(type(), DirectArgumentsType, ptr));

could this max() be spectre trained in a way that hurts us?

> Source/WTF/wtf/MathExtras.h:492
> +inline unsigned preciseIndexMaskShiftForSize(unsigned size)
> +{
> +    return size * 8 - 1;
> +}
> +
> +template<typename T>
> +unsigned preciseIndexMaskShift()
> +{
> +    return preciseIndexMaskShiftForSize(sizeof(T));
> +}

constexpr for both of these?

> Source/WTF/wtf/MathExtras.h:497
> +    asm("" : "+r"(pointer));

asm volatile?

> Source/WTF/wtf/MathExtras.h:516
> +	   (static_cast<uintptr_t>(opaque(actual) ^ expected) << 40));

Is it worth giving this 40 a name since it's hard coded inside JSC in a few
places too?