Bug 181221 – Ensure that poisoned pointers do not look like double or int32 JSValues.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This means that the top 16 bits of any poisoned bits should be 0.

<rdar://problem/36248638>

[Mark Lam]

Created attachment 330344 [details]
proposed patch.

[JF Bastien]

Comment on attachment 330344 [details]
proposed patch.

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

r=me

> Source/WTF/wtf/Poisoned.cpp:40
> +    } while (!(poison >> 32)); // Ensure that bits 32-47 are not completely 0.

You only really need to loop for one of the two calls to cryptographicallyRandomNumber, because the first one's top 16 bits are the only part that are in bits 32-47.

> Source/WTF/wtf/WTFAssertions.cpp:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2018

> Source/WTF/wtf/WTFAssertions.cpp:35
> +struct DummyClass { };

Pedantic pedantry: DummyStruct?

> Source/WTF/wtf/WTFAssertions.cpp:57
> +static_assert(!(makeConstExprPoison(0xffffffff) & 0x3), "ensure bottom 2 alignment bits are available for use as flag bits.");

You don't test bit 2 is set.

> Source/WTF/wtf/WTFAssertions.cpp:58
> +

Do you have tests equivalent to these but for the non-constexpr version?

[Mark Lam]

Comment on attachment 330344 [details]
proposed patch.

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

>> Source/WTF/wtf/Poisoned.cpp:40
>> +    } while (!(poison >> 32)); // Ensure that bits 32-47 are not completely 0.
> 
> You only really need to loop for one of the two calls to cryptographicallyRandomNumber, because the first one's top 16 bits are the only part that are in bits 32-47.

The second term can also contribute non-zero bits to bits 32-47.  Plus this function is only executed a finite number of times ever.  So, perf is not a relevant factor here.

>> Source/WTF/wtf/WTFAssertions.cpp:2
>> + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
> 
> 2018

2017-2018 because this files was renamed from PointerAsserts.cpp which was introduced in 2017.

>> Source/WTF/wtf/WTFAssertions.cpp:35
>>  struct DummyClass { };
> 
> Pedantic pedantry: DummyStruct?

Sure but I'll do that in a subsequent patch since it is not directly related to this one.

>> Source/WTF/wtf/WTFAssertions.cpp:57
>> +static_assert(!(makeConstExprPoison(0xffffffff) & 0x3), "ensure bottom 2 alignment bits are available for use as flag bits.");
> 
> You don't test bit 2 is set.

Sure, I'll add those asserts just to be pedantic.

>> Source/WTF/wtf/WTFAssertions.cpp:58
>> +
> 
> Do you have tests equivalent to these but for the non-constexpr version?

See body of makePoison().  I have 2 RELEASE_ASSERTs.  The other 2 conditions are explicitly spelled out in the code already.  So, I didn't assert those 2.

[Mark Lam]

Created attachment 330348 [details]
patch for landing w/ wincairo build fix.

[Mark Lam]

Thanks for the review.  Landed in r226344: <http://trac.webkit.org/r226344>.