WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
181221
Ensure that poisoned pointers do not look like double or int32 JSValues.
https://bugs.webkit.org/show_bug.cgi?id=181221
Summary
Ensure that poisoned pointers do not look like double or int32 JSValues.
Mark Lam
Reported
2018-01-02 14:49:52 PST
This means that the top 16 bits of any poisoned bits should be 0. <
rdar://problem/36248638
>
Attachments
proposed patch.
(18.01 KB, patch)
2018-01-02 15:23 PST
,
Mark Lam
jfbastien
: review+
Details
Formatted Diff
Diff
patch for landing w/ wincairo build fix.
(18.54 KB, patch)
2018-01-02 16:06 PST
,
Mark Lam
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Mark Lam
Comment 1
2018-01-02 15:23:44 PST
Created
attachment 330344
[details]
proposed patch.
JF Bastien
Comment 2
2018-01-02 15:38:09 PST
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 3
2018-01-02 15:50:09 PST
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
Comment 4
2018-01-02 16:06:36 PST
Created
attachment 330348
[details]
patch for landing w/ wincairo build fix.
Mark Lam
Comment 5
2018-01-02 16:57:58 PST
Thanks for the review. Landed in
r226344
: <
http://trac.webkit.org/r226344
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug