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

improperly detecting a closed wayland socket? #201

Open
nazarewk opened this issue Oct 23, 2023 · 3 comments
Open

improperly detecting a closed wayland socket? #201

nazarewk opened this issue Oct 23, 2023 · 3 comments

Comments

@nazarewk
Copy link

nazarewk commented Oct 23, 2023

TLDR; Seems to me like 84f16d4 introduced very simple and in some cases ineffective detection of Wayland socket availability by comparing FD numbers.

I have observed it when trying to add a shortcut in KDE Plasma that didn't work at all. It took me many hours to find out what was wrong with help of others on KDE devs channel (here is gist I posted).
The message I found in the logs did not make any sense to me because I expected wl-copy to be interacting ONLY with stdin and a wayland socket and those 2 were quite obviously available in the script pasted below:

script content
pass show kwallet | wl-copy -n

notify-send --expire-time=10000 --wait "KWallet password" "is available in clipboard until this notificaton closes"

for i in {1..10} ; do
  echo "password cleared $i times" | wl-copy
done
wl-copy --clear

The guys on the channel pointed me at 84f16d4 introducing some kind of open wayland socket validation. I was not aware it was possible to run a linux program without stdin/out/err connected, but it makes perfect sense in cases there is nothing to output logs to.

Requiring /dev/null connected from outside the wl-clipboard doesn't seem like a proper failure mode due to supposed nature of wl-clipboard operations.

In my opinion wl-clipboard should either:

  1. narrow the checks to:
    • wl-paste to stdout
    • wl-copy to stdin and only in case input is not provided on the command line
  2. not check open FDs at all until required to interact with and then report errors

seems related to:

nazarewk added a commit to nazarewk/plasma-manager that referenced this issue Oct 23, 2023
@nazarewk nazarewk changed the title wl-clipboard improperly detects a closed wayland socket? improperly detecting a closed wayland socket? Oct 23, 2023
nazarewk added a commit to nazarewk/plasma-manager that referenced this issue Oct 23, 2023
nazarewk added a commit to nazarewk/plasma-manager that referenced this issue Oct 23, 2023
@bugaevc
Copy link
Owner

bugaevc commented Oct 23, 2023

Hi!

Seems to me like 84f16d4 introduced very simple and in some cases ineffective detection of Wayland socket availability by comparing FD numbers

What that check does is not detect Wayland socket availability (we know the socket is available if wl_display_connect() has returned non-null); rather, it checks for closed stdio fds. The Wayland socket just happens to be the first file descriptor wl-copy creates (excluding all the ones libc uses internally before invoking main()), so that's what we check against STDERR_FILENO.

It took me many hours to find out what was wrong with help of others on KDE devs channel

Which channel was that on, and can you please point me to the logs of the conversation? 🙂

I was not aware it was possible to run a linux program without stdin/out/err connected, but it makes perfect sense in cases there is nothing to output logs to.

It is easily possible, and at the same time absolutely wrong to do that.

A huge lot of software, if not all of it, is written under assumption that it can write logs / diagnostic messages / errors to stderr (and stdout), and also that it can open file descriptors (files, connections), and one won't interfere with the other. As an example of what can go wrong in this case, your warning message that you write to stderr may unexpectedly get sent over a Wayland connection socket. Or in our case, we closed/replaced stdout, and that accidentally closed the Wayland connection socket, disconnecting us, and then a different file got the same fd, and we were getting a cryptic error trying to sendmsg() on it.

It is really not OK to launch a process with stdio fds closed, other than in very specific circumstances (for example, I would expect PID 1 to be extra careful around stdio fds early on, although Linux does launch PID 1 with stdio fds connected to the console). Doing this is a bug in the software launching the process, in this case in KDE. Do KDE developers disagree? (I assume you are not a KDE developer yourself, please tell me if that's wrong and you're speaking on behalf of KDE.)

Requiring /dev/null connected from outside the wl-clipboard doesn't seem like a proper failure mode due to supposed nature of wl-clipboard operations.

We don't exactly require /dev/null, you're free to provide any file descriptors you want (as long as they're readable/writable as appropriate, won't unexpectedly block or sigpipe the process, and so on), but you must pass something, not a closed fd. We're also not requiring anything additional over what most other software equally expects, with 84f16d4 we just got better at detecting when this fundamental assumption is violated, and failing fast in that case.

wl-clipboard is really just the messenger here; lots of other software must be also (subtly?) broken by this KDE issue. You're saying yourself that you didn't realize this case was even possible; surely all the software you ever wrote doesn't attempt to handle it either.

In my opinion wl-clipboard should either:

  1. narrow the checks to:
    • wl-paste to stdout
    • wl-copy to stdin and only in case input is not provided on the command line

Well, no, because:

  • both tools use stderr for outputting various diagnostics
  • wl-copy actually closes its stdout
  1. not check open FDs at all until required to interact with and then report errors

That is hardly possible, considering that messages may come from wl-clipboard itself, or from inside libc, or from inside libwayland-client. In any case, I don't see why we would want to do that, it's better to detect the issue upfront, so you can report it to KDE people, which you have done, so the check has exactly served its purpose 😃

@nazarewk
Copy link
Author

nazarewk commented Oct 23, 2023

It took me many hours to find out what was wrong with help of others on KDE devs channel

Which channel was that on, and can you please point me to the logs of the conversation? 🙂
on KDE Developers matrix, here is the start, but this is few hours after I gathered information about it to make sense.

Actually it was a great idea to fall back to both TTY and syslog, I would not find the issue at all otherwise.

Reading through your reply I think improving the message should suffice. As a developer not very familiar with inner workings of the Desktop Environments I find following information missing/misleading:

  • which/how many file descriptors are missing? current message suggests just one
  • what can I do about it?
  • all combined the wording seems somehow lazy and generic: This is a bug in the software that has launched wl-clipboard. Aborting

I'd like to see something like:

- wl-clipboard has been launched with a closed standard file descriptor.
+ Some of the standard file descriptors were not passed down to wl-clipboard, but all 3 are required for it's proper operation.
- This is a bug in the software that has launched wl-clipboard.
+ This is a bug in one of the parent programs launching wl-clipboard.
+ You can try fixing the issue by running wl-clipboard through systemd-cat or redirecting to/from /dev/null yourself.
Aborting.

@bugaevc
Copy link
Owner

bugaevc commented Oct 23, 2023

on KDE Developers matrix, here is the start, but this is few hours after I gathered information about it to make sense.

Eh, unfortunately it refuses to show me the past messages, so I cannot see anything; but thank you nevertheless.

Reading through your reply I think improving the message should suffice.

We can absolutely improve the error message, sure, and thanks for the suggestions. (But we have to keep in mind that while the message should also be reasonably concise.)

which/how many file descriptors are missing? current message suggests just one

I don't think the specific number is important, but indeed the current message seems to suggest that it's just one. How about this?

- wl-clipboard has been launched with a closed standard file descriptor.
+ wl-clipboard has been launched with some standard file descriptors closed.

what can I do about it?

Report it to the developers of the software that has launched wl-clipboard, KDE in this case; or fix the bug if you are a KDE developer reproducing this. This is what the "This is a bug in the software that has launched wl-clipboard." blurb is meant to suggest. Do you perhaps have any ideas how we could make this more clear?

You can try fixing the issue by running wl-clipboard through systemd-cat or redirecting to/from /dev/null yourself.

That, while possible, is a partial workaround rather than a fix. As said, this not only affects wl-clipboard, it also affects that systemd-cat you're using there too, your shell that runs the script, every other command you spawn there, and so on. wl-clipboard is just one piece of software among those affected that knows to detect this and report it to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants