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

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dominik Inführ]

[ARM, MIPS] Enable pointer poisoning also for 32-bit architectures

[Dominik Inführ]

Created attachment 335195 [details]
Patch

[Mark Lam]

Comment on attachment 335195 [details]
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)".

[Dominik Inführ]

Created attachment 338419 [details]
Patch

[Dominik Inführ]

So thanks for the review! I hope I've incorporated your feedback now, the patch now uses bit 1 as poison mark bit.

I've run all tests on ARM/MIPS with this diff to make sure that no pointer has bit 1 set:

--- a/Source/WTF/wtf/Poisoned.h
+++ b/Source/WTF/wtf/Poisoned.h
@@ -195,7 +195,10 @@ private:
     constexpr static PoisonedBits poison(const Poisoned*, std::nullptr_t) { return 0; }
 #if ENABLE(POISON)
     template<typename U>
-    ALWAYS_INLINE static PoisonedBits poison(const Poisoned* thisPoisoned, U ptr) { return ptr ? bitwise_cast<PoisonedBits>(ptr) ^ Poison::key(thisPoisoned) : 0; }
+    ALWAYS_INLINE static PoisonedBits poison(const Poisoned* thisPoisoned, U ptr) {
+        RELEASE_ASSERT(!(bitwise_cast<PoisonedBits>(ptr) & 0x2));
+        return ptr ? bitwise_cast<PoisonedBits>(ptr) ^ Poison::key(thisPoisoned) : 0;
+    }
     template<typename U = T>
     ALWAYS_INLINE static U unpoison(const Poisoned* thisPoisoned, PoisonedBits poisonedBits) { return poisonedBits ? bitwise_cast<U>(poisonedBits ^ Poison::key(thisPoisoned)) : bitwise_cast<U>(0ll); }
 #else

Not sure whether to add this assertion as ASSERT for JSVALUE32_64.

[Dominik Inführ]

ping

[Dominik Inführ]

After disabling pointer poisoning by default (https://bugs.webkit.org/show_bug.cgi?id=185586), is this patch still interesting for JSC?

[Maciej Stachowiak]

Comment on attachment 338419 [details]
Patch

Poisoning hs been removed.