Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prefer XDG_RUNTIME_DIR over /tmp for the socket #2222

Merged
merged 3 commits into from Aug 3, 2019

Conversation

@kugel-
Copy link
Member

kugel- commented Jul 18, 2019

$XDG_RUNTIME_DIR is the place where to put socket files these days.

It seems good practice to create a sub-directory as well, perhaps we
create more files there in the future.

The fallback to write in /tmp remains for the rare occasions where
$XDG_RUNTIME_DIR cannot be used.

$XDG_RUNTIME_DIR is the place where to put socket files these days.

It seems good practice to create a sub-directory as well, perhaps we
create more files there in the future.

The fallback to write in /tmp remains for the rare occasions where
$XDG_RUNTIME_DIR cannot be used.
@kugel-

This comment has been minimized.

Copy link
Member Author

kugel- commented Jul 18, 2019

One other use case: I want to be able to bind-mount the socket directory into a container to open files from within the container in the Geany instance on the host, but I do not want to share /tmp with the container.

@elextr

This comment has been minimized.

Copy link
Member

elextr commented Jul 18, 2019

A good idea, but the implementation without the random number on the socket name means that a user can't open two Geanys with differing config files because they each will try to open the same socket.

Copy link
Member

b4n left a comment

I like the idea, but I'm a little worried about what the spec says on lifetime of files in that directory:
Files in this directory MAY be subjected to periodic clean-up. To ensure that your files are not removed, they should have their access time timestamp modified at least once every 6 hours of monotonic time or the 'sticky' bit should be set on the file.

This suggests our socket link could disappear on us after 6 hours if we're not careful, doesn't it? That could be a problem and the source of weird bugs.

* Directory Specification, see
* https://specifications.freedesktop.org/basedir-spec/latest */
real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL);
if (g_mkdir_with_parents(real_dir, 0755) == 0)

This comment has been minimized.

Copy link
@b4n

b4n Jul 18, 2019

Member

The spec says that The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700. The lifetime of the directory MUST be bound to the user being logged in. It MUST be created when the user first logs in […]. This tells us first that we don't need the with_parents() part as the runtime dir must already exist, and then even if it didn't have to it must be created with mode 0700, not 0755.
I suggest we use g_mkdir(real_dir, 0700), so we only create it when it's "safe".
Also note that g_get_user_runtime_dir() is slightly annoying in that it returns $XDG_CACHE_HOME if $XDG_RUNTIME_DIR isn't set, and that directory does not have cleanup semantics similar to /tmp or $XDG_RUNTIME_DIR.

This comment has been minimized.

Copy link
@kugel-

kugel- Jul 18, 2019

Author Member

I use _with_parents() in case the geany directory already exists. I could also use mkdir() and handle EEXIST but this seemed more convenient.

For the runtime directory, I assume it already exists, since it's provided by the spec. If that's not the case I think we cuoldn't create it since users generally don't have sufficient permissions for /var/run/user.

I also considered that the 0700 requirement applies to the $XDG_RUNTIME_DIR itself. A quick survey on my system shows that the subdirectories are inconsistent, but the systemd subdirectory (and systemd manages $XDG_RUNTIME_DIR) has 0755. I guess it doesn't matter much. Can change to 0700 if you want.

srw-rw-rw- 1 kugel kugel   0 16. Jul 06:57 bus
drwx------ 2 kugel kugel  60 17. Jul 16:14 dconf
drwxr-xr-x 2 kugel kugel  40 18. Jul 07:13 geany
drwx------ 2 kugel kugel 140 16. Jul 06:57 gnupg
srw------- 1 kugel kugel   0 16. Jul 06:57 kdeinit5__0
srwxr-xr-x 1 kugel kugel   0 16. Jul 06:57 klauncherdKXvtl.1.slave-socket
-rw-r--r-- 1 kugel kugel  74 16. Jul 06:57 KSMserver__0
srwxr-xr-x 1 kugel kugel   0 16. Jul 06:57 kwallet5.socket
drwxr-xr-x 2 kugel kugel  60 16. Jul 06:57 Nextcloud
drwxr-xr-x 2 kugel kugel  60 16. Jul 06:57 p11-kit
srw-rw-rw- 1 kugel kugel   0 16. Jul 06:57 pipewire-0
drwx------ 2 kugel kugel 100 16. Jul 06:57 pulse
drwxr-xr-x 3 kugel kugel 100 16. Jul 06:57 systemd
drwx------ 2 kugel kugel  40 18. Jul 21:56 xpra
@kugel-

This comment has been minimized.

Copy link
Member Author

kugel- commented Jul 18, 2019

Good point. I didn't find exact information unfortunately. Just this one SO discussion: https://unix.stackexchange.com/questions/356302/sticky-bit-and-socket-files-in-xdg-runtime-dir

That suggests that 1) the 6h requirement applies to real files (and I tend to follow since the spec makes a distinction between files and file objects in the same paragraph), and 2) no implementation actually implements the periodic cleanup (which is a MAY requirement).

On the other hand, the spec guarantees that $XDG_RUNTIME_DIR itself exists until the last logout.

So given that and that the spec expressively tells to put sockets in that directory, I would say it's not an issue to worry about.

@kugel-

This comment has been minimized.

Copy link
Member Author

kugel- commented Jul 19, 2019

@b4n do you still request changes? If yes, which exactly?

We don't want to create $XDG_RUNTIME_DIR and parents, they ought to exist.
utils_mkdir is slightly more convinient than g_mkdir().

Also, keep the mechanism to give the socket a unique filename, as requested
by Colomban (@b4n).
@b4n
b4n approved these changes Jul 28, 2019
Copy link
Member

b4n left a comment

Apart from the nitpicking below, LGTM

src/socket.c Outdated Show resolved Hide resolved
src/socket.c Outdated Show resolved Hide resolved
@b4n

This comment has been minimized.

Copy link
Member

b4n commented Jul 28, 2019

[on periodic cleanup]

OK fair enough. It'd actually be kind of strange to clean up sockets (but who knows). Let's just hope it's OK for now then, and not worry about it further.

@kugel-

This comment has been minimized.

Copy link
Member Author

kugel- commented Jul 29, 2019

@b4n Thanks for the review. I'll squash-merge this soon if you approve.

src/socket.c Outdated Show resolved Hide resolved
@b4n
b4n approved these changes Jul 30, 2019
Copy link
Member

b4n left a comment

LGTM

@elextr
elextr approved these changes Jul 30, 2019
@elextr

This comment has been minimized.

Copy link
Member

elextr commented Jul 30, 2019

@b4n, yes there is a small chance of a clash but its so small ... or the process id could be included or proper UUIDs but hey, I doubt its worth it.

@kugel- kugel- merged commit 21b06d1 into geany:master Aug 3, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kugel- kugel- deleted the kugel-:xdg_runtime branch Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.