[Webkit-unassigned] [Bug 183407] [ARM, MIPS] Enable pointer poisoning also for 32-bit architectures

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183407

Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #335195|review?                     |review-
              Flags|                            |

--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 335195
  --> https://bugs.webkit.org/attachment.cgi?id=335195
Patch

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

r- because this patch hasn't solved where to put the poison marker bit, which is necessary for correctness.

> Source/WTF/wtf/Poisoned.cpp:45
> +#elif USE(JSVALUE32_64)

Just make this #else.  We only have JSVALUE64 or JSVALUE32_64.  There won't be a 3rd value representation.

> Source/WTF/wtf/Poisoned.cpp:46
> +    poison = static_cast<uintptr_t>(cryptographicallyRandomNumber() << 3);

This is insufficient.  Look at the code below:

    poison |= (1 << 2);
    RELEASE_ASSERT(!(poison & 0x3)); // Read the comment in the code that accompanies this assert.

This line sets the 3rd bit from the bottom.  This only makes sense for 64-bit pointers which are 8 byte aligned.  32-bit pointers are 4 byte aligned.  Hence, you'll have collision with pointer bits here.  Note: we set the 3rd bit because we're reserving the bottom 2 bits for clients who may put flag bits down there.  I forgot the case that motivated this but you should be able to do svn blame on this line to find the commit that introduced it.  One of the pointers being poisoned in that patch uses the low bits for flags.

In order to fix this, you need to solve this issue in a different way for 32-bit CPUs.  Is there a bit that is never used for normal valid pointers?  If so, that's the bit you can use for your poison marker.  Otherwise, you'll have to invent some scheme to solve this issue.

> Source/WTF/wtf/Poisoned.h:200
> +    ALWAYS_INLINE static U unpoison(const Poisoned* thisPoisoned, PoisonedBits poisonedBits) { return poisonedBits ? bitwise_cast<U>(poisonedBits ^ Poison::key(thisPoisoned)) : bitwise_cast<U>(intptr_t(0)); }

While I personally like this syntax, I think it is not the WebKit style of doing cast.  You should change this to "static_cast<intptr_t>(0)".

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180307/48a3ff36/attachment.html>