Bug 235177 – Fix WTF::UUID's potential collision with empty and deleted values

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Fix WTF::UUID's potential collision with empty and deleted values

[Yusuke Suzuki]

Created attachment 449042 [details]
Patch

[Yusuke Suzuki]

Created attachment 449043 [details]
Patch

[Anders Carlsson]

Wouldn't it be better to instead have WTF:UUID generate Version 4 UUIDs and maybe use two reserved UUIDs as the empty and deleted values?

https://en.wikipedia.org/wiki/Universally_unique_identifier#Version_4_(random)

[Yusuke Suzuki]

(In reply to Anders Carlsson from comment #3)
> Wouldn't it be better to instead have WTF:UUID generate Version 4 UUIDs and
> maybe use two reserved UUIDs as the empty and deleted values?
> 
> https://en.wikipedia.org/wiki/
> Universally_unique_identifier#Version_4_(random)

For now, I would like to just fix the serious bug which can corrupt our hash table in this patch.

[Chris Dumez]

Comment on attachment 449043 [details]
Patch

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

> Source/WTF/wtf/UUID.cpp:48
>      cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);

Could we do something like this instead of hard-coding 2?
```
do {
    cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);
} while (m_data == emptyValue || m_data == deletedValue);
```

> Source/WTF/wtf/UUID.h:106
> +    encoder << static_cast<uint64_t>(m_data >> 64) << static_cast<uint64_t>(m_data);

Why is this better?

> Source/WTF/wtf/UUID.h:122
> +    return UUID { (static_cast<UInt128>(*high) << 64) | *low };

ditto.

[Yusuke Suzuki]

Comment on attachment 449043 [details]
Patch

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

>> Source/WTF/wtf/UUID.cpp:48
>>      cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);
> 
> Could we do something like this instead of hard-coding 2?
> ```
> do {
>     cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(&m_data), 16);
> } while (m_data == emptyValue || m_data == deletedValue);
> ```

Sounds good. Changed.

>> Source/WTF/wtf/UUID.h:106
>> +    encoder << static_cast<uint64_t>(m_data >> 64) << static_cast<uint64_t>(m_data);
> 
> Why is this better?

UInt128High64 / UInt128Low64 are implementation's detail function.
UInt128 is attempting to support native data structure. And using these implementation detail function prevents us from removing these emulation layer when all compiler supports 128bit native integer primitives.

>> Source/WTF/wtf/UUID.h:122
>> +    return UUID { (static_cast<UInt128>(*high) << 64) | *low };
> 
> ditto.

Ditto. It is implementation detail function, which should not be used by user code.

[Yusuke Suzuki]

Created attachment 449076 [details]
Patch

[Chris Dumez]

Comment on attachment 449076 [details]
Patch

r=me

[Chris Dumez]

Are we landing this?

[Yusuke Suzuki]

(In reply to Chris Dumez from comment #9)
> Are we landing this?

I was waiting for the complete of all EWS, and it finished. So now, landing...

[EWS]

Committed r288020 (246046@main): <https://commits.webkit.org/246046@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/87606347>

[Simon Fraser (smfr)]

Comment on attachment 449076 [details]
Patch

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

> Source/WTF/ChangeLog:10
> +        and used, it can break hash tables. This patch avoids that by picking 2 when we generate 0 or 1.

Seems to not match the code? It just generates another one.

[Yusuke Suzuki]

Comment on attachment 449076 [details]
Patch

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

>> Source/WTF/ChangeLog:10
>> +        and used, it can break hash tables. This patch avoids that by picking 2 when we generate 0 or 1.
> 
> Seems to not match the code? It just generates another one.

Ah, I forgot updating it while changing the implementation based on Chris's comment.