flatpak / flatpak-xdg-utils

Simple portal-based commandline tools for use inside flatpak sandboxes
GNU Lesser General Public License v2.1
34 stars 14 forks source link

(flatpak-spawn) Unable to read struct signalfd_siginfo: Bad file descriptor #55

Open Erick555 opened 3 years ago

Erick555 commented 3 years ago

Linux distribution and version

Arch Linux

Flatpak version

Flatpak 1.11.2

Description of the problem

While building flatpaks using flatpak-builder flatpak there are a lot (hundreds) of warnings related to flatpak-spawn printed in output at various stages:

** (flatpak-spawn:29): WARNING **: 11:28:11.436: Unable to read struct signalfd_siginfo: Bad file descriptor
...
** (flatpak-spawn:59): WARNING **: 11:37:10.183: Unable to read struct signalfd_siginfo: Bad file descriptor
...
** (flatpak-spawn:78): WARNING **: 11:45:53.263: Unable to read struct signalfd_siginfo: Bad file descriptor
...
** (flatpak-spawn:101): WARNING **: 11:45:59.814: Unable to read struct signalfd_siginfo: Bad file descriptor

Beside of the above the build succeeds.

Steps to reproduce

Build flatpak using flatpak-builder flatpak, i.e flatpak run org.flatpak.Builder build org.flatpak.Builder.yml

smcv commented 3 years ago

Is this running in org.freedesktop.Sdk runtime, version 20.08?

This seems like it's probably a bug in the version of https://github.com/flatpak/flatpak-xdg-utils in whatever runtime you're using.

smcv commented 3 years ago

Unable to read struct signalfd_siginfo: Bad file descriptor

This would indicate that the signalfd has somehow become invalid, perhaps because it got overwritten by another file descriptor.

smcv commented 3 years ago

Actually, I think this might be a bug in https://github.com/flathub/org.flatpak.Builder/blob/master/fusermount-wrapper.sh. It's blindly forwarding fd 3 to the host, even if fd 3 is not actually open. If fd 3 is the signalfd that flatpak-spawn uses internally, then it'll get closed, leading to failure.

Ideally flatpak-spawn would validate the fds that it receives on its command-line, and check that they're all things that were open on startup and are not being used internally.

Erick555 commented 3 years ago

Is this running in org.freedesktop.Sdk runtime, version 20.08?

Yes

@smcv should I open issue in org.flatpak.Builder repo at flathub then or it has to be fixed here?

nanonyme commented 3 years ago

@smcv looks like that file dates back to beginning of the repo in 2018. Is there a spec somewhere of how the fd handling is actually supposed to work like? Can there be more fd's than 1, 2 and 3? Would it work to go through /proc/self/fd/?

smcv commented 3 years ago

Is there a spec somewhere of how the fd handling is actually supposed to work like?

fds 0, 1 and 2 (stdin, stdout and stderr respectively) are always forwarded, so forwarding those explicitly is unnecessary.

Any other file descriptor should only be forwarded if it is both: (a) already open, and (b) necessary to forward.

If you don't know whether a file descriptor is necessary to forward, then it isn't. They should only be necessary to forward if there is some "protocol" or "API" that tells the child process which file descriptors it should be reading or writing.

For example, imagine you are running an instance of dbus-daemon on the host (for some reason). If you are using --print-address=23 --print-pid=42, which instruct it to print its address on fd 23 and its pid on fd 42, then you will need to forward fds 23 and 42, otherwise that won't work. The command-line options are a "protocol" for telling the dbus-daemon which fds to use.

It looks as though when libfuse runs fusermount, it sets the _FUSE_COMMFD environment variable to the fd number of an inherited fd that is used to communicate back to libfuse. That's a different "protocol" for telling fusermount which fd to use. That's why --forward-fd=${_FUSE_COMMFD} is necessary.

For a very complicated version of this, look at pressure-vessel, which forwards a lot of fds (I would guess a 2-digit number of them) through pressure-vessel-launch (which is basically a modified flatpak-spawn), each with an argument like --forward-fd=23, and an accompanying argument to the program to be run in the sandbox to tell it what it's meant to use the forwarded fd for.

The default in historical Unix (and therefore in shells) is that every fd is inherited unless explicitly flagged to not be inherited.

The default in flatpak-spawn is that every fd is not "inherited" (really forwarded) unless explicitly flagged. With hindsight, this would have been a better default for Unix too, but we're about 40 years too late to fix that.

Can there be more fd's than 1, 2 and 3?

Yes, up to hundreds of thousands on modern systems, but typically somewhere in the range 3 to 100.

Would it work to go through /proc/self/fd/?

I think that would be wrong, because the shell will have fds open for its own purposes that are not intended to be inherited by (flatpak-spawn and therefore) fusermount.

smcv commented 3 years ago

What I would do [edit: to resolve flathub/org.flatpak.Builder#42] is:

Try removing all the --forward-fd, except for --forward-fd=${_FUSE_COMMFD}. It'll probably work.

If it doesn't work, then investigate what fd 3 is/was, and why it is (sometimes? always?) necessary.

smcv commented 3 years ago

Meanwhile, let's use this issue to represent the thing that is wrong in flatpak-spawn:

Ideally flatpak-spawn would validate the fds that it receives on its command-line, and check that they're all things that were open on startup and are not being used internally.

nanonyme commented 3 years ago

@smcv what should it do on user error? (incorrect error passes) Should it fail?

smcv commented 2 years ago

what should it do on user error? (incorrect [file descriptor passed]) Should it fail?

If fd 3 is not a valid fd at startup, then I'm not sure whether --forward-fd=3 should fail with an error message, or be ignored with a warning message. Either one could make sense. What it certainly should not do is forward an unrelated fd that is used internally.