[webkit-reviews] review granted: [Bug 224889] Improve our constructDeletedValue() template specializations : [Attachment 426738] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 224889: Improve our constructDeletedValue() template specializations
https://bugs.webkit.org/show_bug.cgi?id=224889

Attachment 426738: Patch

https://bugs.webkit.org/attachment.cgi?id=426738&action=review




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 426738
  --> https://bugs.webkit.org/attachment.cgi?id=426738
Patch

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

I’m going to say review+; this is really hard to review.

>>>>> Source/WebCore/dom/MessagePortIdentifier.h:103
>>>>> +    static void constructDeletedValue(WebCore::MessagePortIdentifier&
slot) { new (NotNull, &slot.processIdentifier)
WebCore::ProcessIdentifier(WTF::HashTableDeletedValue); }
>>>> 
>>>> safeToCompareToEmptyOrDeleted is true. emptyValue is 0 and deletedValue is
std::numeric_limits<uint64_t>::max() so this is OK.
>>> 
>>> I am initializing the exact same data member as before so no additional
crash risk AFAICT.
>> 
>> Well you're right that you don't make this any worse than it was before, but
the preexisting code is unsafe. The operator== first compares
processIdentifier, then it compares portIdentifier. If both
MessagePortIdentifierHash objects being compared are HashTableDeletedValue,
operator== will read uninitialized data. That may be unlikely but IMO we should
think harder about this one. We could either change operator== to consider only
processIdentifier in case it matches HashTableDeletedValue, or change
constructDeletedValue to also initialize portIdentifier.
>> 
>> If we assume that two HashTableDeletedValues should never be compared, then
it would be safe. But that doesn't seem very robust.
> 
> Looks to me that what you are worried about (2 HashTableDeletedValues being
compared) would only happen when trying to lookup/store a
HashTableDeletedValues in a HashMap. My answer is that this is not something
that is supported.
> Also, even if the 2 would not necessarily compute as equal due to
uninitialized memory, it would not crash. But again, AFAIK (and looking at the
HashTable implementation), this is a non-issue.

Chris is right; we don’t need to support comparing two deleted values with each
other, even if safeToCompareToEmptyOrDeleted is true.

>>>> Source/WebKitLegacy/mac/History/BinaryPropertyList.cpp:77
>>>> +	  static void constructDeletedValue(IntegerArray& slot) {
HashTraits<size_t>::constructDeletedValue(slot.m_size); }
>>> 
>>> safeToCompareToEmptyOrDeleted is true. emptyValue calls the default
constructor of IntegerArray, which sets m_integers & m_size to 0. Deleted value
uses HashTraits<size_t>::constructDeletedValue() for m_size, which ends up
being std::numeric_limits<size_t>::max() so this should be safe to compare.
>> 
>> m_integers is newly uninitialized for the deleted value. operator== looks
like so:
>> ```
>> inline bool operator==(const IntegerArray& a, const IntegerArray& b)
>> {
>>     return a.m_integers == b.m_integers &&  a.m_size == b.m_size;
>> }
>> ```
>> 
>> Even though m_integers is now uninitialized, this is doing a pure pointer
comparison and should not crash AFAIK.
> 
> Well sure, this function *itself* won't crash, but this is the same case as
GlobalWindowIdentifier: the comparison function checks the uninitialized value
first, so it's *always* comparing uninitialized memory when comparing any
normal IntegerArray against a HashTableDeletedValue IntegerArray.

Not sure it’s a real problem. Depends on what kind of debugging tool we are
using. Outside of debugging tools, there are no magic "uninitialized" values
that can make integer comparison unsafe.