Bug 124756 – Ensure that arity fixups honor stack alignment requirements

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 217638 [details]
the patch.

[Mark Lam]

Comment on attachment 217638 [details]
the patch.

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:60
> +    // However, to simplify the calcultation, we'll assume the caller has allocated none

typo: "calcultation" => "calculation".  Will fix before landing.

[Geoffrey Garen]

Comment on attachment 217638 [details]
the patch.

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:65
> +    int neededStackIncrease = newCodeBlock->numParameters() - virtualRegisterForLocal(newCodeBlock->m_numCalleeRegisters).offset() + stackAlignmentRegisters();

No need to add in m_numCalleeRegisters. The function entry point we return to will check that for us. Let's focus this function on just our arguments.

It's not good to use one variable to check the stack limit and a separate variable to hold the actual stack change we're going to make, with the two variables having different values. We want our stack check to check the value we're actually going to use. Otherwise, we introduce the possibility of a security bug, and code that's harder to reason about. If you checked the actual value you were going to use, you wouldn't need a long comment explaining how this fake value correlates to the actual value.

How about this:

ASSERT(argumentCountIncludingThis < newCodeBlock->numParameters());
int missingArgumentCount = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), newCodeBlock->numParameters() - argumentCountIncludingThis);
if (!stack->grow(exec->registers() - (missingArgumentCount * sizeof(Register)))
    // FAIL
if (!vm.isSafeToRecurse(missingArgumentCount * sizeof(Register)))
    // FAIL
return missingArgumentCount;

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:71
> +    // The caller may or may not have already allocated some space for the incoming args.
> +    // However, to simplify the calcultation, we'll just conservatively allocate space
> +    // for the expected number of parameters + the number of callee registers. In addition,
> +    // we'll conservatively add the amount of 

Please remove.

> Source/JavaScriptCore/runtime/VM.h:376
> +            return (&curr - neededStackInBytes) >= m_stackLimit;

This calculation will underflow if neededStackInBytes is too big. You need

    &curr >= m_stackLimit && &curr - m_stackLimit >= neededStackInBytes

[Geoffrey Garen]

> How about this:
> 
> ASSERT(argumentCountIncludingThis < newCodeBlock->numParameters());
> int missingArgumentCount = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), newCodeBlock->numParameters() - argumentCountIncludingThis);

Actually, this is not quite right, because you also need to account for the size of the call frame header. (It is not guaranteed to be an aligned number.) So, you need:

size_t alignedArgumentCount = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), newCodeBlock->numParameters() + CallFrameHeaderSize) - CallFrameHeaderSize;
ASSERT(argumentCountIncludingThis < alignedArgumentCount);
return alignedArgumentCount - argumentCountIncludingThis;

[Mark Lam]

(In reply to comment #4)
> > How about this:
> > 
> > ASSERT(argumentCountIncludingThis < newCodeBlock->numParameters());
> > int missingArgumentCount = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), newCodeBlock->numParameters() - argumentCountIncludingThis);
> 
> Actually, this is not quite right, because you also need to account for the size of the call frame header. (It is not guaranteed to be an aligned number.) So, you need:
> 
> size_t alignedArgumentCount = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), newCodeBlock->numParameters() + CallFrameHeaderSize) - CallFrameHeaderSize;
> ASSERT(argumentCountIncludingThis < alignedArgumentCount);
> return alignedArgumentCount - argumentCountIncludingThis;

Hmmm, I don't think that that is needed.  Consider this example:

1. Upon entry to this code, the callFrame pointer is guaranteed to be aligned, independent on how many bytes the CallFrameHeader is.
2. Alignment requires 16 bytes.  Let's say we need 8 bytes for 1 more slot.  We'll on need to move the frame by a delta of 16 (rounded up from 8).

Regardless of the size of the CallFrameHeader, because we're guaranteed that the callFrame pointer is aligned by the time we get here, all we have to do is ensure that the delta we are introducing is also padded to the required alignment because that's how many bytes we'll shift the frame by.  The callFrame pointer will remain aligned after the shift regardless of the size of the CallFrameHeader.

[Mark Lam]

Created attachment 217705 [details]
patch 2: addressed Geoff's feedback.

[Geoffrey Garen]

Comment on attachment 217705 [details]
patch 2: addressed Geoff's feedback.

r=me

[Mark Lam]

Thanks.  Landed in r159706: <http://trac.webkit.org/r159706>.