Thread: 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 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. Also recently reported as a bug: https://postgr.es/m/17358-89806e7420797025%40postgresql.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: https://postgr.es/m/17358-89806e7420797025%40postgresql.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
Attachment
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"));
+#endif
Older 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
Attachment
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