Bug 182086 – DirectArguments should protect itself using dynamic poisoning and precise index masking

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 332233 [details]
maybe the patch

[Filip Pizlo]

Created attachment 332319 [details]
the patch

[Saam Barati]

Comment on attachment 332319 [details]
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?

[Filip Pizlo]

(In reply to Saam Barati from comment #3)
> Comment on attachment 332319 [details]
> 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?

No.  At worst, you'll get a smaller limit.

> 
> > 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?

I'll try that.

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

I don't need it to be 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?

I'll do that.

[Filip Pizlo]

Created attachment 332327 [details]
patch for landing

[Radar WebKit Bug Importer]

<rdar://problem/36887421>

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/227643/webkit

[Mark Lam]

Comment on attachment 332327 [details]
patch for landing

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

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1049
> +    UNUSED_PARAM(actualValue);

This is a typo, and as a result, you're breaking the 32-bit builds.

[Mark Lam]

Comment on attachment 332327 [details]
patch for landing

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

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1049
>> +    UNUSED_PARAM(actualValue);
> 
> This is a typo, and as a result, you're breaking the 32-bit builds.

Fixed 32-bit build in r227644: <https://trac.webkit.org/r227644>.

[Matt Lewis]

This also broke the windows build.

Looks like its a syntax error and another error:

MathExtras.h(497): error C2059: syntax error: ':' 

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/7382

[Filip Pizlo]

(In reply to Matt Lewis from comment #10)
> This also broke the windows build.
> 
> Looks like its a syntax error and another error:
> 
> MathExtras.h(497): error C2059: syntax error: ':' 
> 
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 7382

Fixed in https://trac.webkit.org/changeset/227648/webkit