Improve our constructDeletedValue() template specializations

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Improve our constructDeletedValue() template specializations.

[Chris Dumez]

Created attachment 426738 [details]
Patch

[Chris Dumez]

Created attachment 426745 [details]
Patch

[Darin Adler]

To review this we need to know safeToCompareToEmptyOrDeleted values for each class. If it’s true then we need to audit the == functions with the deleted value in mind.

[Chris Dumez]

Comment on attachment 426738 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBKeyData.h:209
> +    static void constructDeletedValue(IDBKeyData& key) { key.m_isDeletedValue = true; }

safeToCompareToEmptyOrDeleted is false for IDBKeyData.

> Source/WebCore/Modules/indexeddb/shared/IDBResourceIdentifier.h:111
>      static void constructDeletedValue(IDBResourceIdentifier& identifier)

safeToCompareToEmptyOrDeleted is false.

> Source/WebCore/PAL/pal/SessionID.h:118
> +    static void constructDeletedValue(PAL::SessionID& slot) { new (NotNull, &slot) PAL::SessionID(HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is true but I did not change behavior here.

> 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.

> Source/WebCore/history/BackForwardItemIdentifier.h:117
> +    static void constructDeletedValue(WebCore::BackForwardItemIdentifier& slot) { new (NotNull, &slot.processIdentifier) WebCore::ProcessIdentifier(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so this is OK.

> Source/WebCore/layout/LayoutUnits.h:247
> +    static void constructDeletedValue(WebCore::Layout::SlotPosition& slot) { slot.column = std::numeric_limits<size_t>::max(); }

safeToCompareToEmptyOrDeleted is true. Empty value is column=0/row=std::numeric_limits<size_t>::max(). Deleted value is column=std::numeric_limits<size_t>::max() so they cannot be equal.

[Chris Dumez]

Comment on attachment 426738 [details]
Patch

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

> Source/WebCore/loader/PrivateClickMeasurement.h:525
> +    static void constructDeletedValue(WebCore::PrivateClickMeasurement::SourceSite& slot) { new (NotNull, &slot.registrableDomain) WebCore::RegistrableDomain(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is false.

> Source/WebCore/loader/PrivateClickMeasurement.h:532
> +    static void constructDeletedValue(WebCore::PrivateClickMeasurement::AttributionDestinationSite& slot) { new (NotNull, &slot.registrableDomain) WebCore::RegistrableDomain(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is false.

> Source/WebCore/page/ClientOrigin.h:105
> +    static void constructDeletedValue(WebCore::ClientOrigin& slot) { new (NotNull, &slot.topOrigin) WebCore::SecurityOriginData(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is false.

> Source/WebCore/page/GlobalWindowIdentifier.h:97
> +    static void constructDeletedValue(WebCore::GlobalWindowIdentifier& slot) { new (NotNull, &slot.windowIdentifier) WebCore::WindowIdentifier(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is true. Empty value is 0, DeletedValue is std::numeric_limits<uint64_t>::max() so comparison is safe.

> Source/WebCore/platform/Cookie.h:172
> +        static void constructDeletedValue(WebCore::Cookie& slot) { new (NotNull, &slot.name) String(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is false.

> Source/WebCore/platform/graphics/FontCache.h:210
> +    static void constructDeletedValue(FontCascadeCacheKey& slot) { new (NotNull, &slot.fontDescriptionKey) FontDescriptionKey(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is false.

> Source/WebCore/platform/graphics/IntPointHash.h:38
> +    static void constructDeletedValue(WebCore::IntPoint& slot) { slot.setX(std::numeric_limits<int>::min()); }

safeToCompareToEmptyOrDeleted is true. Empty value is x=0/y=std::numeric_limits<int>::min(). Deleted value is x=std::numeric_limits<int>::min()/y=0 so comparison is safe.

> Source/WebCore/rendering/CSSValueKey.h:92
> +    static void constructDeletedValue(WebCore::CSSValueKey& slot) { new (NotNull, &slot) WebCore::CSSValueKey { WebCore::CSSValueInvalid, true, true}; }

safeToCompareToEmptyOrDeleted is true. Empty value is using false for the booleans while deleted value is using true for the booleans. Comparison is safe and I did not change behavior.

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:112
> +    static void constructDeletedValue(WebCore::ServiceWorkerClientIdentifier& slot) { new (NotNull, &slot.serverConnectionIdentifier) WebCore::SWServerConnectionIdentifier(HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so comparison is safe.

> Source/WebKit/NetworkProcess/cache/NetworkCache.h:86
> +    static void constructDeletedValue(WebKit::NetworkCache::GlobalFrameID& slot) { new (NotNull, &slot.webPageID) WebCore::PageIdentifier(WTF::HashTableDeletedValue); }

safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so comparison is safe.

> Source/WebKit/Platform/IPC/StringReference.h:106
> +    static void constructDeletedValue(IPC::StringReference& stringReference) { stringReference.m_size = std::numeric_limits<size_t>::max(); }

safeToCompareToEmptyOrDeleted is true. Empty value uses 0 for m_size while deleted value uses std::numeric_limits<size_t>::max() so this is safe to compare.

> Source/WebKit/Shared/CallbackID.h:121
> +    static void constructDeletedValue(WebKit::CallbackID& slot) { HashTraits<uint64_t>::constructDeletedValue(slot.m_id); }

safeToCompareToEmptyOrDeleted is true. empty value is HashTraits<uint64_t>::emptyValue() (so 0) and deleted value is HashTraits<uint64_t>::constructDeletedValue() (so std::numeric_limits<uint64_t>::max()) so this should be safe to compare.

> 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.

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> To review this we need to know safeToCompareToEmptyOrDeleted values for each
> class. If it’s true then we need to audit the == functions with the deleted
> value in mind.

I had this in mind when I wrote the patch but I did another pass to be safe and added comments on the patch to facilitate review.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> I had this in mind when I wrote the patch but I did another pass to be safe
> and added comments on the patch to facilitate review.

Yes, looks great.

To state the obvious, the two kinds of unsafe to worry about would be a crash when doing the comparison or a false positive where it would compare as equal. Most of your comments are about the "false positive" half.

[Chris Dumez]

Comment on attachment 426738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426738&action=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.

>> Source/WebCore/history/BackForwardItemIdentifier.h:117
>> +    static void constructDeletedValue(WebCore::BackForwardItemIdentifier& slot) { new (NotNull, &slot.processIdentifier) WebCore::ProcessIdentifier(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value 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.

>> Source/WebCore/layout/LayoutUnits.h:247
>> +    static void constructDeletedValue(WebCore::Layout::SlotPosition& slot) { slot.column = std::numeric_limits<size_t>::max(); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is column=0/row=std::numeric_limits<size_t>::max(). Deleted value is column=std::numeric_limits<size_t>::max() so they cannot be equal.

row is newly uninitialized for the deleted value and operator== looks like so:
```
inline bool operator==(const SlotPosition& a, const SlotPosition& b)
{
    return a.column == b.column && a.row == b.row;
}
```

Those are size_t types though and AFAIK, even if uninitialized, doing a comparison should not cause a crash.

>> Source/WebCore/page/GlobalWindowIdentifier.h:97
>> +    static void constructDeletedValue(WebCore::GlobalWindowIdentifier& slot) { new (NotNull, &slot.windowIdentifier) WebCore::WindowIdentifier(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is 0, DeletedValue is std::numeric_limits<uint64_t>::max() so comparison is safe.

I am initialized the exact same data member as before so no new crash risk AFAICT.

>> Source/WebCore/platform/graphics/IntPointHash.h:38
>> +    static void constructDeletedValue(WebCore::IntPoint& slot) { slot.setX(std::numeric_limits<int>::min()); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is x=0/y=std::numeric_limits<int>::min(). Deleted value is x=std::numeric_limits<int>::min()/y=0 so comparison is safe.

y is newly uninitialized for the deleted value. operator== looks like so:
```
inline bool operator==(const IntPoint& a, const IntPoint& b)
{
    return a.x() == b.x() && a.y() == b.y();
}
```

The data members have int type and comparison should not crash even if uninitialized.

>> Source/WebCore/rendering/CSSValueKey.h:92
>> +    static void constructDeletedValue(WebCore::CSSValueKey& slot) { new (NotNull, &slot) WebCore::CSSValueKey { WebCore::CSSValueInvalid, true, true}; }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is using false for the booleans while deleted value is using true for the booleans. Comparison is safe and I did not change behavior.

No new risk of crash AFAIK because this is still doing a full initialization.

>> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:112
>> +    static void constructDeletedValue(WebCore::ServiceWorkerClientIdentifier& slot) { new (NotNull, &slot.serverConnectionIdentifier) WebCore::SWServerConnectionIdentifier(HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so comparison is safe.

No new risk of crash before I am initializing the exact same data member as before.

>> Source/WebKit/NetworkProcess/cache/NetworkCache.h:86
>> +    static void constructDeletedValue(WebKit::NetworkCache::GlobalFrameID& slot) { new (NotNull, &slot.webPageID) WebCore::PageIdentifier(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so comparison is safe.

No new risk of crash before I am initializing the exact same data member as before.

>> Source/WebKit/Platform/IPC/StringReference.h:106
>> +    static void constructDeletedValue(IPC::StringReference& stringReference) { stringReference.m_size = std::numeric_limits<size_t>::max(); }
> 
> safeToCompareToEmptyOrDeleted is true. Empty value uses 0 for m_size while deleted value uses std::numeric_limits<size_t>::max() so this is safe to compare.

m_data is newly uninitialized in the deleted value case. operator== looks like so:
```
inline bool operator==(const StringReference& a, const StringReference& b)
{
    return a.size() == b.size() && !memcmp(a.data(), b.data(), a.size());
}
```

The sizes would not match so we wouldn't do a memcmp(). The memcpy() would be equally bad before or after my change.

>> Source/WebKit/Shared/CallbackID.h:121
>> +    static void constructDeletedValue(WebKit::CallbackID& slot) { HashTraits<uint64_t>::constructDeletedValue(slot.m_id); }
> 
> safeToCompareToEmptyOrDeleted is true. empty value is HashTraits<uint64_t>::emptyValue() (so 0) and deleted value is HashTraits<uint64_t>::constructDeletedValue() (so std::numeric_limits<uint64_t>::max()) so this should be safe to compare.

No new risk of crash as far as I can tell before I am initializing the exact same data member as before.

>> 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.

[Chris Dumez]

Anyone wants to review this? I tried to facilitate the review as much as possible by adding comments. This patch was inspired by Darin's feedback here:
- https://bugs.webkit.org/show_bug.cgi?id=224755#c34
- https://bugs.webkit.org/show_bug.cgi?id=224755#c47

[Michael Catanzaro]

Comment on attachment 426738 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBKeyData.h:209
>> +    static void constructDeletedValue(IDBKeyData& key) { key.m_isDeletedValue = true; }
> 
> safeToCompareToEmptyOrDeleted is false for IDBKeyData.

Good.

>> Source/WebCore/Modules/indexeddb/shared/IDBResourceIdentifier.h:111
>>      static void constructDeletedValue(IDBResourceIdentifier& identifier)
> 
> safeToCompareToEmptyOrDeleted is false.

Good.

>> Source/WebCore/PAL/pal/SessionID.h:118
>> +    static void constructDeletedValue(PAL::SessionID& slot) { new (NotNull, &slot) PAL::SessionID(HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is true but I did not change behavior here.

Good. This one is safe anyway, because you're constructing the entire PAL::SessionID.

>>> 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.

>>> Source/WebCore/history/BackForwardItemIdentifier.h:117
>>> +    static void constructDeletedValue(WebCore::BackForwardItemIdentifier& slot) { new (NotNull, &slot.processIdentifier) WebCore::ProcessIdentifier(WTF::HashTableDeletedValue); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value 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.

Same problem, here itemIdentifier is left uninitialized, so uninitialized data will be read when comparing two BackForwardItemIdentifiers that are both HashTableDeletedValue.

>>> Source/WebCore/layout/LayoutUnits.h:247
>>> +    static void constructDeletedValue(WebCore::Layout::SlotPosition& slot) { slot.column = std::numeric_limits<size_t>::max(); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is column=0/row=std::numeric_limits<size_t>::max(). Deleted value is column=std::numeric_limits<size_t>::max() so they cannot be equal.
> 
> row is newly uninitialized for the deleted value and operator== looks like so:
> ```
> inline bool operator==(const SlotPosition& a, const SlotPosition& b)
> {
>     return a.column == b.column && a.row == b.row;
> }
> ```
> 
> Those are size_t types though and AFAIK, even if uninitialized, doing a comparison should not cause a crash.

Again, it works except when comparing two HashTableDeletedValues, then we read uninitialized data.

>> Source/WebCore/loader/PrivateClickMeasurement.h:525
>> +    static void constructDeletedValue(WebCore::PrivateClickMeasurement::SourceSite& slot) { new (NotNull, &slot.registrableDomain) WebCore::RegistrableDomain(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is false.

Good. Nice cleanup with this one.

>> Source/WebCore/loader/PrivateClickMeasurement.h:532
>> +    static void constructDeletedValue(WebCore::PrivateClickMeasurement::AttributionDestinationSite& slot) { new (NotNull, &slot.registrableDomain) WebCore::RegistrableDomain(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is false.

Good.

>> Source/WebCore/page/ClientOrigin.h:105
>> +    static void constructDeletedValue(WebCore::ClientOrigin& slot) { new (NotNull, &slot.topOrigin) WebCore::SecurityOriginData(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is false.

Good.

>>> Source/WebCore/page/GlobalWindowIdentifier.h:97
>>> +    static void constructDeletedValue(WebCore::GlobalWindowIdentifier& slot) { new (NotNull, &slot.windowIdentifier) WebCore::WindowIdentifier(WTF::HashTableDeletedValue); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is 0, DeletedValue is std::numeric_limits<uint64_t>::max() so comparison is safe.
> 
> I am initialized the exact same data member as before so no new crash risk AFAICT.

Well it is not worse than the preexisting code, but it's not safe. This one is actually the worst so far because the comparison function checks the uninitialized value first, so it's *always* comparing uninitialized memory when comparing any normal GlobalWindowIdentifier against a HashTableDeletedValue GlobalWindowIdentifier. The other cases I complain about here only matter when comparing two HashTableDeletedValues, which maybe we don't do, but this one is clearly bad regardless. So please reconsider this one.

>> Source/WebCore/platform/Cookie.h:172
>> +        static void constructDeletedValue(WebCore::Cookie& slot) { new (NotNull, &slot.name) String(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is false.

Good.

>> Source/WebCore/platform/graphics/FontCache.h:210
>> +    static void constructDeletedValue(FontCascadeCacheKey& slot) { new (NotNull, &slot.fontDescriptionKey) FontDescriptionKey(WTF::HashTableDeletedValue); }
> 
> safeToCompareToEmptyOrDeleted is false.

Good.

>>> Source/WebCore/platform/graphics/IntPointHash.h:38
>>> +    static void constructDeletedValue(WebCore::IntPoint& slot) { slot.setX(std::numeric_limits<int>::min()); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is x=0/y=std::numeric_limits<int>::min(). Deleted value is x=std::numeric_limits<int>::min()/y=0 so comparison is safe.
> 
> y is newly uninitialized for the deleted value. operator== looks like so:
> ```
> inline bool operator==(const IntPoint& a, const IntPoint& b)
> {
>     return a.x() == b.x() && a.y() == b.y();
> }
> ```
> 
> The data members have int type and comparison should not crash even if uninitialized.

It works except when comparing two HashTableDeletedValues. (I expect safeToCompareToEmptyOrDeleted means "it will always work properly and not cause valgrind to complain" and not "it won't crash.")

>>> Source/WebCore/rendering/CSSValueKey.h:92
>>> +    static void constructDeletedValue(WebCore::CSSValueKey& slot) { new (NotNull, &slot) WebCore::CSSValueKey { WebCore::CSSValueInvalid, true, true}; }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is using false for the booleans while deleted value is using true for the booleans. Comparison is safe and I did not change behavior.
> 
> No new risk of crash AFAIK because this is still doing a full initialization.

I agree, this one is safe. Good.

>>> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:112
>>> +    static void constructDeletedValue(WebCore::ServiceWorkerClientIdentifier& slot) { new (NotNull, &slot.serverConnectionIdentifier) WebCore::SWServerConnectionIdentifier(HashTableDeletedValue); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so comparison is safe.
> 
> No new risk of crash before I am initializing the exact same data member as before.

Again, it works except when comparing two HashTableDeletedValues, then we read uninitialized data.

>>> Source/WebKit/NetworkProcess/cache/NetworkCache.h:86
>>> +    static void constructDeletedValue(WebKit::NetworkCache::GlobalFrameID& slot) { new (NotNull, &slot.webPageID) WebCore::PageIdentifier(WTF::HashTableDeletedValue); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value is 0, deleted value is std::numeric_limits<uint64_t>::max() so comparison is safe.
> 
> No new risk of crash before I am initializing the exact same data member as before.

Again, it works except when comparing two HashTableDeletedValues, then we read uninitialized data.

>>> Source/WebKit/Platform/IPC/StringReference.h:106
>>> +    static void constructDeletedValue(IPC::StringReference& stringReference) { stringReference.m_size = std::numeric_limits<size_t>::max(); }
>> 
>> safeToCompareToEmptyOrDeleted is true. Empty value uses 0 for m_size while deleted value uses std::numeric_limits<size_t>::max() so this is safe to compare.
> 
> m_data is newly uninitialized in the deleted value case. operator== looks like so:
> ```
> inline bool operator==(const StringReference& a, const StringReference& b)
> {
>     return a.size() == b.size() && !memcmp(a.data(), b.data(), a.size());
> }
> ```
> 
> The sizes would not match so we wouldn't do a memcmp(). The memcpy() would be equally bad before or after my change.

Again, it works except when comparing two HashTableDeletedValues, then we read uninitialized data.

>>> Source/WebKit/Shared/CallbackID.h:121
>>> +    static void constructDeletedValue(WebKit::CallbackID& slot) { HashTraits<uint64_t>::constructDeletedValue(slot.m_id); }
>> 
>> safeToCompareToEmptyOrDeleted is true. empty value is HashTraits<uint64_t>::emptyValue() (so 0) and deleted value is HashTraits<uint64_t>::constructDeletedValue() (so std::numeric_limits<uint64_t>::max()) so this should be safe to compare.
> 
> No new risk of crash as far as I can tell before I am initializing the exact same data member as before.

This one is safe because m_id is the only data member checked by operator==. Good.

>>> 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.

[Chris Dumez]

Comment on attachment 426738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426738&action=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.

[Darin Adler]

Comment on attachment 426738 [details]
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.

[EWS]

Committed r276502 (236960@main): <https://commits.webkit.org/236960@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426738 [details].

[Michael Catanzaro]

(In reply to Darin Adler from comment #12)
> 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.

It's a real problem because the operator== is not going to work properly. The changes to GlobalWindowIdentifier and IntegerArray are not safe. Let's look at IntegerArray:

inline bool operator==(const IntegerArray& a, const IntegerArray& b)
{
    return a.m_integers == b.m_integers &&  a.m_size == b.m_size;
}

Let's say a is a normal IntegerArray and b is a HashTableDeletedValue. Then b.m_integers is uninitialized data. The first comparison will *probably* fail and return false, but we could just get unlucky. Comparing m_size first would avoid this. A similar change would fix GlobalWindowIdentifier. Alternatively, safeToCompareToEmptyOrDeleted should be changed to false.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #14)
> (In reply to Darin Adler from comment #12)
> > 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.
> 
> It's a real problem because the operator== is not going to work properly.
> The changes to GlobalWindowIdentifier and IntegerArray are not safe. Let's
> look at IntegerArray:
> 
> inline bool operator==(const IntegerArray& a, const IntegerArray& b)
> {
>     return a.m_integers == b.m_integers &&  a.m_size == b.m_size;
> }
> 
> Let's say a is a normal IntegerArray and b is a HashTableDeletedValue. Then
> b.m_integers is uninitialized data. The first comparison will *probably*
> fail and return false, but we could just get unlucky. Comparing m_size first
> would avoid this. A similar change would fix GlobalWindowIdentifier.
> Alternatively, safeToCompareToEmptyOrDeleted should be changed to false.

What you just said does not seem to demonstrate that operator== is not going to work properly. Yes, when comparing a valid "a" and a deleted "b", then m_integers might match. That's fine because m_size won't.

[Michael Catanzaro]

OK, it's unlikely that m_size would match, that's true.

[Darin Adler]

(In reply to Michael Catanzaro from comment #16)
> OK, it's unlikely that m_size would match, that's true.

If it’s "unlikely" we have a problem. It needs to be impossible. We must make sure there is no way we can allocate an array that big. If we don’t prevent that, then it’s not a suitable deleted value for the hash table and could even create a problem like a security vulnerability.

I *think* it’s impossible here, but we need to verify that.

[Darin Adler]

I think it’s impossible because we would exhaust memory allocating the m_integers.

[Darin Adler]

(In reply to Chris Dumez from comment #15)
> (In reply to Michael Catanzaro from comment #14)
> > (In reply to Darin Adler from comment #12)
> > > 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.

I meant scalar comparison, not integer comparison.

[Radar WebKit Bug Importer]

<rdar://problem/77110024>