configure --with-uuid=bsd fails on NetBSD
Our documentation claims that --with-uuid=bsd works on both
FreeBSD and NetBSD: installation.sgml says
<option>bsd</option> to use the UUID functions found in FreeBSD, NetBSD,
and some other BSD-derived systems
and there is comparable wording in uuid-ossp.sgml.
In the course of setting up a NetBSD buildfarm animal, I discovered
that this is a lie. NetBSD indeed has the same uuid_create() function
as FreeBSD, but it produces version-4 UUIDs not version-1, which causes
the contrib/uuid-ossp regression tests to fail. You have to dig down
a level to the respective uuidgen(2) man pages to find documentation
about this, but each system appears to be conforming to its docs,
and the old DCE standard they both refer to conveniently omits saying
anything about what kind of UUID you get. So this isn't a bug as
far as either BSD is concerned.
I'm not personally inclined to do anything about this; I'm certainly
not excited enough about it to write our own v1-UUID creation code.
Perhaps we should just document that on NetBSD, uuid_generate_v1()
and uuid_generate_v1mc() don't conform to spec.
regards, tom lane
Hi,
On 2022-08-20 19:39:32 -0400, Tom Lane wrote:
Our documentation claims that --with-uuid=bsd works on both
FreeBSD and NetBSD: installation.sgml says<option>bsd</option> to use the UUID functions found in FreeBSD, NetBSD,
and some other BSD-derived systemsand there is comparable wording in uuid-ossp.sgml.
In the course of setting up a NetBSD buildfarm animal, I discovered
that this is a lie.
Also recently reported as a bug: /messages/by-id/17358-89806e7420797025@postgresql.org
with a bunch of discussion.
I'm not personally inclined to do anything about this; I'm certainly
not excited enough about it to write our own v1-UUID creation code.
Perhaps we should just document that on NetBSD, uuid_generate_v1()
and uuid_generate_v1mc() don't conform to spec.
Perhaps we should make them error out instead? It doesn't seem helpful to
just return something wrong...
Certainly would be good to get the regression tests to pass somehow, given
that we don't expect this to work. Don't want to add netbsd's results as an
alternative, because that'd maybe hide bugs. But if we errored out we could
probably have an alternative with the errors, without a large risk of hiding
bugs.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2022-08-20 19:39:32 -0400, Tom Lane wrote:
In the course of setting up a NetBSD buildfarm animal, I discovered
that this is a lie.
Also recently reported as a bug: /messages/by-id/17358-89806e7420797025@postgresql.org
with a bunch of discussion.
Ah, I'd totally forgotten that thread :-(. After Peter pointed
to the existence of new UUID format proposals, I kind of lost
interest in doing a lot of work to implement our own not-quite-
per-spec V1 generator.
Perhaps we should make them error out instead? It doesn't seem helpful to
just return something wrong...
Yeah, might be appropriate.
regards, tom lane
Hi,
On 8/21/22 04:37, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
Perhaps we should make them error out instead? It doesn't seem helpful to
just return something wrong...Yeah, might be appropriate.
Based on these discussions, I attached a patch.
Thanks,
Nazir Bilal Yavuz
Attachments:
v1-netbsd-error-out-uuid_generate_v1-and-uuid_generate_v1mc-functions.patchtext/x-patch; charset=UTF-8; name=v1-netbsd-error-out-uuid_generate_v1-and-uuid_generate_v1mc-functions.patchDownload
diff --git a/contrib/uuid-ossp/expected/uuid_ossp_1.out b/contrib/uuid-ossp/expected/uuid_ossp_1.out
new file mode 100644
index 0000000000..4b3d61388c
--- /dev/null
+++ b/contrib/uuid-ossp/expected/uuid_ossp_1.out
@@ -0,0 +1,111 @@
+CREATE EXTENSION "uuid-ossp";
+SELECT uuid_nil();
+ uuid_nil
+--------------------------------------
+ 00000000-0000-0000-0000-000000000000
+(1 row)
+
+SELECT uuid_ns_dns();
+ uuid_ns_dns
+--------------------------------------
+ 6ba7b810-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+SELECT uuid_ns_url();
+ uuid_ns_url
+--------------------------------------
+ 6ba7b811-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+SELECT uuid_ns_oid();
+ uuid_ns_oid
+--------------------------------------
+ 6ba7b812-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+SELECT uuid_ns_x500();
+ uuid_ns_x500
+--------------------------------------
+ 6ba7b814-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+-- some quick and dirty field extraction functions
+-- this is actually timestamp concatenated with clock sequence, per RFC 4122
+CREATE FUNCTION uuid_timestamp_bits(uuid) RETURNS varbit AS
+$$ SELECT ('x' || substr($1::text, 15, 4) || substr($1::text, 10, 4) ||
+ substr($1::text, 1, 8) || substr($1::text, 20, 4))::bit(80)
+ & x'0FFFFFFFFFFFFFFF3FFF' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_version_bits(uuid) RETURNS varbit AS
+$$ SELECT ('x' || substr($1::text, 15, 2))::bit(8) & '11110000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_reserved_bits(uuid) RETURNS varbit AS
+$$ SELECT ('x' || substr($1::text, 20, 2))::bit(8) & '11000000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_multicast_bit(uuid) RETURNS bool AS
+$$ SELECT (('x' || substr($1::text, 25, 2))::bit(8) & '00000001') != '00000000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_local_admin_bit(uuid) RETURNS bool AS
+$$ SELECT (('x' || substr($1::text, 25, 2))::bit(8) & '00000010') != '00000000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_node(uuid) RETURNS text AS
+$$ SELECT substr($1::text, 25) $$
+LANGUAGE SQL STRICT IMMUTABLE;
+-- Ideally, the multicast bit would never be set in V1 output, but the
+-- UUID library may fall back to MC if it can't get the system MAC address.
+-- Also, the local-admin bit might be set (if so, we're probably inside a VM).
+-- So we can't test either bit here.
+SELECT uuid_version_bits(uuid_generate_v1()),
+ uuid_reserved_bits(uuid_generate_v1());
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+-- Although RFC 4122 only requires the multicast bit to be set in V1MC style
+-- UUIDs, our implementation always sets the local-admin bit as well.
+SELECT uuid_version_bits(uuid_generate_v1mc()),
+ uuid_reserved_bits(uuid_generate_v1mc()),
+ uuid_multicast_bit(uuid_generate_v1mc()),
+ uuid_local_admin_bit(uuid_generate_v1mc());
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+-- timestamp+clock sequence should be monotonic increasing in v1
+SELECT uuid_timestamp_bits(uuid_generate_v1()) < uuid_timestamp_bits(uuid_generate_v1());
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+SELECT uuid_timestamp_bits(uuid_generate_v1mc()) < uuid_timestamp_bits(uuid_generate_v1mc());
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+-- Ideally, the node value is stable in V1 addresses, but OSSP UUID
+-- falls back to V1MC behavior if it can't get the system MAC address.
+SELECT CASE WHEN uuid_multicast_bit(uuid_generate_v1()) AND
+ uuid_local_admin_bit(uuid_generate_v1()) THEN
+ true -- punt, no test
+ ELSE
+ uuid_node(uuid_generate_v1()) = uuid_node(uuid_generate_v1())
+ END;
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+-- In any case, V1MC node addresses should be random.
+SELECT uuid_node(uuid_generate_v1()) <> uuid_node(uuid_generate_v1mc());
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+SELECT uuid_node(uuid_generate_v1mc()) <> uuid_node(uuid_generate_v1mc());
+ERROR: NetBSD's uuid_create function generates version-4 UUIDs instead of version-1
+SELECT uuid_generate_v3(uuid_ns_dns(), 'www.widgets.com');
+ uuid_generate_v3
+--------------------------------------
+ 3d813cbb-47fb-32ba-91df-831e1593ac29
+(1 row)
+
+SELECT uuid_generate_v5(uuid_ns_dns(), 'www.widgets.com');
+ uuid_generate_v5
+--------------------------------------
+ 21f7f8de-8051-5b89-8680-0195ef798b6a
+(1 row)
+
+SELECT uuid_version_bits(uuid_generate_v4()),
+ uuid_reserved_bits(uuid_generate_v4());
+ uuid_version_bits | uuid_reserved_bits
+-------------------+--------------------
+ 01000000 | 10000000
+(1 row)
+
+SELECT uuid_generate_v4() <> uuid_generate_v4();
+ ?column?
+----------
+ t
+(1 row)
+
diff --git a/contrib/uuid-ossp/uuid-ossp.c b/contrib/uuid-ossp/uuid-ossp.c
index b868812358..cc74685a34 100644
--- a/contrib/uuid-ossp/uuid-ossp.c
+++ b/contrib/uuid-ossp/uuid-ossp.c
@@ -467,6 +467,10 @@ uuid_ns_x500(PG_FUNCTION_ARGS)
Datum
uuid_generate_v1(PG_FUNCTION_ARGS)
{
+#if defined(__NetBSD__)
+ ereport(ERROR, errmsg("NetBSD's uuid_create function generates "
+ "version-4 UUIDs instead of version-1"));
+#endif
return uuid_generate_internal(UUID_MAKE_V1, NULL, NULL, 0);
}
@@ -474,6 +478,10 @@ uuid_generate_v1(PG_FUNCTION_ARGS)
Datum
uuid_generate_v1mc(PG_FUNCTION_ARGS)
{
+#if defined(__NetBSD__)
+ ereport(ERROR, errmsg("NetBSD's uuid_create function generates "
+ "version-4 UUIDs instead of version-1"));
+#endif
#ifdef HAVE_UUID_OSSP
char *buf = NULL;
#elif defined(HAVE_UUID_E2FS)
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
Based on these discussions, I attached a patch.
This is the wrong way to go about it:
+#if defined(__NetBSD__)
+ ereport(ERROR, errmsg("NetBSD's uuid_create function generates "
+ "version-4 UUIDs instead of version-1"));
+#endifOlder versions of NetBSD generated v1, so you'd incorrectly break
things on those. And who knows whether they might reconsider
in the future?
I think the right fix is to call uuid_create and then actually check
the version field of the result. This avoids breaking what need not
be broken, and it'd also guard against comparable problems on other
platforms (so don't blame NetBSD specifically in the message, either).
regards, tom lane
Hi,
On 8/26/22 19:21, Tom Lane wrote:
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
Based on these discussions, I attached a patch.
I think the right fix is to call uuid_create and then actually check
the version field of the result. This avoids breaking what need not
be broken, and it'd also guard against comparable problems on other
platforms (so don't blame NetBSD specifically in the message, either).
I updated my patch. I checked version field in 'uuid_generate_internal'
function instead of checking it in 'uuid_generate_v1' and
'uuid_generate_v1mc' functions, but I have some questions:
1 - Should it be checked only for '--with-uuid=bsd' option?
1.1 - If it is needed to be checked only for '--with-uuid=bsd',
should just NetBSD be checked?
2 - Should it error out without including current UUID version in the
error message? General error message could mask if the 'uuid_create'
function starts to produce UUIDs other than version-4.
Regards,
Nazir Bilal Yavuz
Attachments:
v2-0001-error-out-uuid_create-function.patchtext/x-patch; charset=UTF-8; name=v2-0001-error-out-uuid_create-function.patchDownload
diff --git a/contrib/uuid-ossp/expected/uuid_ossp_1.out b/contrib/uuid-ossp/expected/uuid_ossp_1.out
new file mode 100644
index 0000000000..e9db9d9b1b
--- /dev/null
+++ b/contrib/uuid-ossp/expected/uuid_ossp_1.out
@@ -0,0 +1,111 @@
+CREATE EXTENSION "uuid-ossp";
+SELECT uuid_nil();
+ uuid_nil
+--------------------------------------
+ 00000000-0000-0000-0000-000000000000
+(1 row)
+
+SELECT uuid_ns_dns();
+ uuid_ns_dns
+--------------------------------------
+ 6ba7b810-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+SELECT uuid_ns_url();
+ uuid_ns_url
+--------------------------------------
+ 6ba7b811-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+SELECT uuid_ns_oid();
+ uuid_ns_oid
+--------------------------------------
+ 6ba7b812-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+SELECT uuid_ns_x500();
+ uuid_ns_x500
+--------------------------------------
+ 6ba7b814-9dad-11d1-80b4-00c04fd430c8
+(1 row)
+
+-- some quick and dirty field extraction functions
+-- this is actually timestamp concatenated with clock sequence, per RFC 4122
+CREATE FUNCTION uuid_timestamp_bits(uuid) RETURNS varbit AS
+$$ SELECT ('x' || substr($1::text, 15, 4) || substr($1::text, 10, 4) ||
+ substr($1::text, 1, 8) || substr($1::text, 20, 4))::bit(80)
+ & x'0FFFFFFFFFFFFFFF3FFF' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_version_bits(uuid) RETURNS varbit AS
+$$ SELECT ('x' || substr($1::text, 15, 2))::bit(8) & '11110000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_reserved_bits(uuid) RETURNS varbit AS
+$$ SELECT ('x' || substr($1::text, 20, 2))::bit(8) & '11000000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_multicast_bit(uuid) RETURNS bool AS
+$$ SELECT (('x' || substr($1::text, 25, 2))::bit(8) & '00000001') != '00000000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_local_admin_bit(uuid) RETURNS bool AS
+$$ SELECT (('x' || substr($1::text, 25, 2))::bit(8) & '00000010') != '00000000' $$
+LANGUAGE SQL STRICT IMMUTABLE;
+CREATE FUNCTION uuid_node(uuid) RETURNS text AS
+$$ SELECT substr($1::text, 25) $$
+LANGUAGE SQL STRICT IMMUTABLE;
+-- Ideally, the multicast bit would never be set in V1 output, but the
+-- UUID library may fall back to MC if it can't get the system MAC address.
+-- Also, the local-admin bit might be set (if so, we're probably inside a VM).
+-- So we can't test either bit here.
+SELECT uuid_version_bits(uuid_generate_v1()),
+ uuid_reserved_bits(uuid_generate_v1());
+ERROR: uuid_create function is not generating version-1 UUIDs
+-- Although RFC 4122 only requires the multicast bit to be set in V1MC style
+-- UUIDs, our implementation always sets the local-admin bit as well.
+SELECT uuid_version_bits(uuid_generate_v1mc()),
+ uuid_reserved_bits(uuid_generate_v1mc()),
+ uuid_multicast_bit(uuid_generate_v1mc()),
+ uuid_local_admin_bit(uuid_generate_v1mc());
+ERROR: uuid_create function is not generating version-1 UUIDs
+-- timestamp+clock sequence should be monotonic increasing in v1
+SELECT uuid_timestamp_bits(uuid_generate_v1()) < uuid_timestamp_bits(uuid_generate_v1());
+ERROR: uuid_create function is not generating version-1 UUIDs
+SELECT uuid_timestamp_bits(uuid_generate_v1mc()) < uuid_timestamp_bits(uuid_generate_v1mc());
+ERROR: uuid_create function is not generating version-1 UUIDs
+-- Ideally, the node value is stable in V1 addresses, but OSSP UUID
+-- falls back to V1MC behavior if it can't get the system MAC address.
+SELECT CASE WHEN uuid_multicast_bit(uuid_generate_v1()) AND
+ uuid_local_admin_bit(uuid_generate_v1()) THEN
+ true -- punt, no test
+ ELSE
+ uuid_node(uuid_generate_v1()) = uuid_node(uuid_generate_v1())
+ END;
+ERROR: uuid_create function is not generating version-1 UUIDs
+-- In any case, V1MC node addresses should be random.
+SELECT uuid_node(uuid_generate_v1()) <> uuid_node(uuid_generate_v1mc());
+ERROR: uuid_create function is not generating version-1 UUIDs
+SELECT uuid_node(uuid_generate_v1mc()) <> uuid_node(uuid_generate_v1mc());
+ERROR: uuid_create function is not generating version-1 UUIDs
+SELECT uuid_generate_v3(uuid_ns_dns(), 'www.widgets.com');
+ uuid_generate_v3
+--------------------------------------
+ 3d813cbb-47fb-32ba-91df-831e1593ac29
+(1 row)
+
+SELECT uuid_generate_v5(uuid_ns_dns(), 'www.widgets.com');
+ uuid_generate_v5
+--------------------------------------
+ 21f7f8de-8051-5b89-8680-0195ef798b6a
+(1 row)
+
+SELECT uuid_version_bits(uuid_generate_v4()),
+ uuid_reserved_bits(uuid_generate_v4());
+ uuid_version_bits | uuid_reserved_bits
+-------------------+--------------------
+ 01000000 | 10000000
+(1 row)
+
+SELECT uuid_generate_v4() <> uuid_generate_v4();
+ ?column?
+----------
+ t
+(1 row)
+
diff --git a/contrib/uuid-ossp/uuid-ossp.c b/contrib/uuid-ossp/uuid-ossp.c
index b868812358..ece46a31d8 100644
--- a/contrib/uuid-ossp/uuid-ossp.c
+++ b/contrib/uuid-ossp/uuid-ossp.c
@@ -282,6 +282,16 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
uuid_to_string(&uu, &str, &status);
if (status == uuid_s_ok)
{
+ /* At newer versions of BSD, uuid_create function
+ * could produce other UUID versions than version-1.
+ * Check version field(14) of generated UUID and
+ * error out if required.
+ */
+ if(str[14] != '1')
+ ereport(ERROR,
+ errmsg("uuid_create function is not "
+ "generating version-1 UUIDs"));
+
strlcpy(strbuf, str, 37);
/*
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
I updated my patch. I checked version field in 'uuid_generate_internal'
function instead of checking it in 'uuid_generate_v1' and
'uuid_generate_v1mc' functions, but I have some questions:
Yeah, that seems like the right place. I tweaked the code to check
strbuf not str just so we aren't making unnecessary assumptions about
the length of what is returned. strbuf[14] is guaranteed to exist,
str[14] maybe not.
1 - Should it be checked only for '--with-uuid=bsd' option?
1.1 - If it is needed to be checked only for '--with-uuid=bsd',
should just NetBSD be checked?
I don't see any reason not to check in the BSD code path --- it's
a cheap enough test. On the other hand, in the other code paths
there is no evidence that it's necessary, and we'd find out soon
enough if it becomes necessary.
2 - Should it error out without including current UUID version in the
error message? General error message could mask if the 'uuid_create'
function starts to produce UUIDs other than version-4.
Yeah, I thought reporting the actual version digit was worth doing,
and made it do so.
Pushed with those changes and doc updates. I did not push the
variant expected-file. I think the entire point here is that
we are *not* deeming the new NetBSD implementation acceptable,
so allowing it to pass regression tests is the wrong thing.
regards, tom lane
Hi,
On 2022-09-09 12:48:38 -0400, Tom Lane wrote:
Pushed with those changes and doc updates. I did not push the
variant expected-file. I think the entire point here is that
we are *not* deeming the new NetBSD implementation acceptable,
so allowing it to pass regression tests is the wrong thing.
What do we gain from the regression test failing exactly this way, given that
we know it's a problem? It just makes it harder to run tests. How about we add
it as variant file, but via the resultmap mechanism? That way we wouldn't
silently accept the same bug on other platforms, but can still run the test
without needing to manually filter out bogus netbsd results?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2022-09-09 12:48:38 -0400, Tom Lane wrote:
Pushed with those changes and doc updates. I did not push the
variant expected-file. I think the entire point here is that
we are *not* deeming the new NetBSD implementation acceptable,
so allowing it to pass regression tests is the wrong thing.
What do we gain from the regression test failing exactly this way, given that
we know it's a problem?
It tells people not to use --with-uuid=bsd on those NetBSD versions.
They can either do without uuid-ossp, or install ossp or e2fs.
("Do without" is not much of a hardship, now that we have
gen_random_uuid() in core.)
IMV a substantial part of the point of the regression tests is to
let end users and/or packagers verify that they have a non-broken
installation. Hiding a problem by making the tests not fail
basically breaks that use-case.
If we had, say, a known openssl security bug that was exposed by our
test cases, would you advocate dumbing down the tests to not expose
the bug?
It just makes it harder to run tests.
Harder for who? AFAICT there is nobody but me routinely running
full tests on NetBSD, else we'd have found this problem much earlier.
I've got my animals configured not to use --with-uuid (not much of
a lift considering that's the buildfarm's default). End of problem.
regards, tom lane
Hi,
On 2022-09-09 17:31:40 -0400, Tom Lane wrote:
Harder for who? AFAICT there is nobody but me routinely running
full tests on NetBSD, else we'd have found this problem much earlier.
Bilal's report was caused by automating testing on netbsd (and openbsd) as
well, as part of the meson stuff. I also occasionally run the tests in a VM.
But as you say:
I've got my animals configured not to use --with-uuid (not much of
a lift considering that's the buildfarm's default). End of problem.
Fair enough.
Greetings,
Andres Freund