Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

util/uuid: replace with util/token, remove libuuid #2833

Merged
merged 1 commit into from
Apr 11, 2021

Conversation

r-c-f
Copy link
Contributor

@r-c-f r-c-f commented Apr 7, 2021

Uses getentropy() when available, otherwise /dev/urandom. Closes #2830

meson.build Outdated Show resolved Hide resolved
util/meson.build Outdated Show resolved Hide resolved
util/uuid.c Outdated

#ifndef HAS_GETENTROPY
static int getentropy(void *buffer, size_t len)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: { should be on the previous line

util/uuid.c Outdated Show resolved Hide resolved
util/uuid.c Outdated Show resolved Hide resolved
util/uuid.c Outdated Show resolved Hide resolved
util/uuid.c Outdated Show resolved Hide resolved
@jbeich
Copy link
Contributor

jbeich commented Apr 8, 2021

Note, DragonFly and NetBSD lack getentropy but have getrandom. OpenBSD has getentropy but lacks getrandom. FreeBSD has both. All BSDs (and Android) have arc4random_buf.

@r-c-f
Copy link
Contributor Author

r-c-f commented Apr 8, 2021

The main benefit to getentropy is the avoidance of file IO, with the possible future benefit of being considered for acceptance into POSIX; if that doesn't matter, static /dev/urandom for everything would be the simpler (if slightly leakier) approach, given that a Wayland compositor is already opening up too many file descriptors to worry about constraints.

@kennylevinsen
Copy link
Member

From the proposed POSIX spec:

NAME

    getentropy -- fill a buffer with random bytes

<< SNIP >>

APPLICATION USAGE

    The intended use of this function is to create a seed for other pseudo-random number generators.


RATIONALE

    The getentropy() function is not a cancellation point. (See [xref to 2.9.5.2 Cancellation Points].)

Even though they use the same source, /dev/urandom appears more portable for now and is in line with what libuuid was doing.

util/uuid.c Outdated Show resolved Hide resolved
util/uuid.c Outdated

if (!urandom) {
if (!(urandom = fopen("/dev/urandom", "r"))) {
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to call wlr_log_errno here to make it clear something wrong happened.

util/uuid.c Outdated Show resolved Hide resolved
@emersion emersion added the breaking Breaking change in public API label Apr 9, 2021
@emersion
Copy link
Member

emersion commented Apr 9, 2021

Marking as a breaking change since this removes WLR_HAS_XDG_FOREIGN.

@emersion
Copy link
Member

emersion commented Apr 9, 2021

Hm, actually, it removes xdg-foreign for features but leaves WLR_HAS_XDG_FOREIGN. We should either keep both or get rid of both.

@emersion
Copy link
Member

emersion commented Apr 9, 2021

We can also get rid of libuuid in CI.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! Can you squash all commits into a single one, and remove the CI dependencies introduced in bf4e2e0 (in .builds/*.yml)?

util/token.c Outdated Show resolved Hide resolved
@emersion
Copy link
Member

Ah, also please remove libuuid from the README.

Use 128-bit hexadecimal string tokens generated with /dev/urandom
instead of UUIDs for xdg-foreign handles, removing the libuuid
dependency. Update readme and CI. Closes swaywm#2830.

build: remove xdg-foreign feature

With no external dependencies required, there's no reason not to always
build it. Remove WLR_HAS_XDG_FOREIGN as well.
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@emersion emersion changed the title util/uuid: use custom generator, remove libuuid util/uuid: replace with util/token, remove libuuid Apr 11, 2021
@emersion emersion merged commit b29ac8f into swaywm:master Apr 11, 2021
@emersion emersion mentioned this pull request Apr 11, 2021
emersion added a commit to swaywm/sway that referenced this pull request Apr 11, 2021
RagnarGrootKoerkamp pushed a commit to RagnarGrootKoerkamp/sway that referenced this pull request Jun 17, 2021
emersion added a commit to emersion/sway that referenced this pull request Jun 23, 2021
References: swaywm/wlroots#2833
(cherry picked from commit 1a72049)
kennylevinsen pushed a commit to swaywm/sway that referenced this pull request Jun 24, 2021
References: swaywm/wlroots#2833
(cherry picked from commit 1a72049)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Breaking change in public API
Development

Successfully merging this pull request may close these issues.

Replace libuuid with a random util func
4 participants