[webkit-reviews] review denied: [Bug 124756] Ensure that arity fixups honor stack alignment requirements : [Attachment 217638] the patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 124756: Ensure that arity fixups honor stack alignment requirements
https://bugs.webkit.org/show_bug.cgi?id=124756

Attachment 217638: the patch.
https://bugs.webkit.org/attachment.cgi?id=217638&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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