The code in _uuid_generate_php() blindly copied from http://php.net/uniqid#65879 does not generate valid version 4 UUIDs. The Wikipedia article on UUIDs simply describes the layout of a version 4 UUID as xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx (http://en.wikipedia.org/wiki/Universally_unique_identifier#Version_4_.28...), which is verified by the original RFC (http://www.ietf.org/rfc/rfc4122.txt) when the following errata are taken into account: http://www.pwg.org/archives/ipp/2013/017477.html

The attached patch fixes this issue.

I have also fixed the "PHP Fatal error: Cannot redeclare uuid_is_valid()" error which occurs if the PECL UUID module is installed.

CommentFileSizeAuthor
_uuid_generate_php__not__UUIDv4__valid-unknown-0.patch1.55 KBsdrycroft
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented

Oh, this is a serious one! I will have to look into this a bit more, to see what implications this would have.

My current thinking is actually to leave it as is, and accept that we've made a mistake violating the specification.
Because if we want to fix this, we would need to provide an upgrade path that updates all previously created UUIDs. But that would break a lot of implementations relying on UUIDs to stay consistent (which is the whole point).

I'll loop in our other maintainers on this as well.

skwashd’s picture

skwashd CreditAttribution: skwashd commented

This is a problem, but that proposed patch looks good to me.

I tested some UUIDs generated using the current implementation with the PECL uuid_is_valid function. The code and results are in a pastebin - http://pastebin.com/zHMpnxX6 (DrupalBin is down?)

We should definitely fix the problem, but I don't think it is as bad @dixon_ thinks it is. Let's just make sure we do it right from now on.

A side benefit form this patch for PECL users is that they'll get the performance boost of the validation logic all happening in C, rather than PHP userspace regex.

As a side note, this will need to be fixed in D8 core too.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented

What I mean is that we'll have to make sure that uuid_is_valid() passes on the old (incorrect) format as well. That way, we would not need to provide an upgrade path.

skwashd’s picture

skwashd CreditAttribution: skwashd commented

Our current PHP userspace implementation of uuid_is_valid() is a simple regex. It was designed to check the basic format of the string, because we don't can what version of UUID is being used. This was by design as the UUID generator is pluggable.

The pastebin link in my previous comment shows the PECL UUID version of uuid_is_valid() validating our existing (broken) UUIDs without any problem.

Given the above I think we just need to document the problem in the older versions - including the 6.x-1.x branch (which is where this code came from).

skwashd’s picture

skwashd CreditAttribution: skwashd commented
Status: Needs review » Fixed

Thanks for that patch. This has been applied in a slightly modified form.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jweowu’s picture

jweowu CreditAttribution: jweowu commented
Issue summary: View changes

Cross-referenced with RFC errata, as the original RFC is actually pretty unclear about the position of the version bits, and consequently doesn't really verify Wikipedia's layout as stated (although it is correct).