[webkit-reviews] review denied: [Bug 109836] [JIT] Memory overwrite by Math object functions : [Attachment 190067] Location of memory overwrite

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Wojciech Bielawski
<w.bielawski at samsung.com>'s request for review:
Bug 109836: [JIT] Memory overwrite by Math object functions
https://bugs.webkit.org/show_bug.cgi?id=109836

Attachment 190067: Location of memory overwrite
https://bugs.webkit.org/attachment.cgi?id=190067&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190067&action=review


> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:109
> -	       storeDouble(src, Address(stackPointerRegister,
-(int)sizeof(double)));
> -	       loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue,
u.asBits.tag) - sizeof(double)), regT1);
> -	       loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue,
u.asBits.payload) - sizeof(double)), regT0);
> +	       push(regT2);
> +	       push(regT2);
> +	       storeDouble(src, Address(stackPointerRegister,
+(int)sizeof(double)));
> +	       loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue,
u.asBits.tag) + sizeof(double)), regT1);
> +	       loadPtr(Address(stackPointerRegister, OBJECT_OFFSETOF(JSValue,
u.asBits.payload) + sizeof(double)), regT0);
>	       Jump lowNonZero = branchTestPtr(NonZero, regT1);
>	       Jump highNonZero = branchTestPtr(NonZero, regT0);
> +	       pop(regT2);
> +	       pop(regT2);

This looks wrong; you seem to be jumping over the pops.  This will leave the
stack in a weird state.

I'm not sure if this is a real bug, either.  It ought to be safe to store at a
negative stack offset, in most platforms.

Plus, I don't like the implications for performance of the thunk: you're doing
some extra pushes and pops.

But if it's a real bug, you could have implemented this by changing the offset
at which we store the double.  The JITStackFrame::args should be reusable here,
since we're not in the middle of a JITStub call.