Socket gets unlinked when children closes the socket file descriptor

[aszlig]

strace output provided by @riedel in #13 (comment):

https://gist.github.com/riedel/043936e3c80da0f2607d48d8205d63e0#file-rsession-strace-L3960

My answer:

Okay, so the interesting part here is that it happens prior to executing /bin/sh -c 'git "--version"', where all file descriptors are closed. However, since after the clone the file descriptor is still active in the parent, we should not unlink the socket file.

To fix this, we need to implement some kind of reference counting... oh geesh...

Originally posted by @aszlig in #13 (comment)

[aszlig]

Some more thoughts on this:

The naive way to solve this would be to just wrap clone/clone3 (except if using CLONE_FILES or CLONE_VM), daemon and fork/vfork and make sure the the socket file descriptors controlled by ip2unix aren't unlinked when close is called in the child process.

This will probably work with the rsession case mentioned/reported above, since the clone call is followed up by execve (after closing the file descriptors ranging from 3 to 4095) and thus the socket is not really being used in the child process.

However, this falls short as soon as the child process is actively using the socket and it's closed in the parent, for example (in pseudocode):

fd = socket(...);
pid = fork();
if (pid == 0) { // children
    accept(fd, ...);
    ...
}
// parent
close(fd);
waitpid(pid, ...);

In this case, the socket would be unlinked in the parent way too early and would leave users baffled on why there is no socket file. Note that unlinking the socket doesn't cause any errors in the socket interface and it's a bit similar to our own blackhole rule action.

We could get around this by having a side-channel which keeps track of the processes using a socket controlled by us. One very error-prone, non-portable and racy way would be by checking /proc/PID/net/unix of the parent and/or other processes in the same pgroup. Another way would be to fork off a dedicated ip2unix process that's solely responsible for keeping track of socket file descriptors.

Both however doesn't sound like the right solution here and I'd rather have a more simple solution where we even wouldn't need to wrap fork or clone.

Another way would be to not unlink socket files at all or even add an option to allow the user to control the behaviour. Unfortunately, especially the former would constitute a backwards-incompatible change and I personally would also prefer not to have countless of old socket files laying around.

[riedel]

I was trying to reproduce the bug reliably on centos:7 with glibc-2.17-307.el7.1.x86_64 using docker. One really needs to install git for the problem to appear (so it gets executed with all the sockets closed)

FROM centos:7
RUN yum install -y epel-release centos-release-scl
RUN yum install -y meson yaml-cpp-devel python3-pytest R devtoolset-7-gcc-c++ git
RUN yum install -y https://download2.rstudio.org/server/centos6/x86_64/rstudio-server-rhel-1.3.959-x86_64.rpm
COPY . /ip2unix
WORKDIR /ip2unix
RUN scl enable devtoolset-7 'meson build && ninja -C build && ninja -C build install'
#RUN ninja -C build test
RUN adduser testuser
ENV LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib64
USER testuser
RUN R_HOME=/usr/lib64/R  R_SHARE_DIR=/usr/share/R R_INCLUDE_DIR=/usr/include/R R_DOC_DIR=/usr/share/doc/R-3.6.0 RSTUDIO_DEFAULT_R_VERSION_HOME=/usr/lib64/R  RSTUDIO_DEFAULT_R_VERSION=3.6.0 timeout 10 ip2unix -r path=/tmp/rsession.sock /usr/lib/rstudio-server/bin/rsession --standalone=1 --program-mode=server --log-stderr=1 --www-address=127.0.0.1 --www-port=12345 & sleep 3 && curl --unix-socket /tmp/rsession.sock http://127.0.0.1:12345

[aszlig]

@riedel: Just to make sure my assumptions are correct:

If you uncomment the following line:

ip2unix/src/socket.cc

Line 352 in b54905a

this->unlink_sockpath = newpath;

... does it work?

[riedel]

... does it work?
yes!

[aszlig]

Comment moved to #23.

[ghost]

... does it work?
yes!

Hi, I can report that I got bitten by this bug and the fix above works for me too. Please please consider adding a command-line switch to enable this behavior for users who need it! Leaking socket inodes is better than not working at all.

Specifically, firefox has a low-level debugger protocol called "marionette" which only knows how to listen on TCP sockets. This is a lower-level and more powerful protocol than "chrome devtools" or "webdriver" (in fact, those are implemented using marionette). I don't want to leave such a powerful debugger interface to every random process on the same machine as the browser (including, potentially, website javascripts running inside the browser itself!) and ip2unix does a great job at that -- but it needs this patch. Details: https://bugzilla.mozilla.org/show_bug.cgi?id=1525126#c71

[aszlig]

Please please consider adding a command-line switch to enable this behavior for users who need it! Leaking socket inodes is better than not working at all.

The issue is that adding a command line switch doesn't necessarily make the problem go away, because people do not necessarily know that they need such a switch, so I think we need to have a better interim solution.

One that I could think of would be to unlink the socket file during bind instead of close, which would avoid some shady command line flag that we need to deprecate in version 3. We already do this whenever SO_REUSEADDR is set for the socket.

Long-term however I think we need to switch to fork+execve+wait instead of just execve right now, which we need to do anyway if we want to use seccomp BPF. This model would leave ip2unix running for the whole runtime of the program in question but the above issue would not apply because we can also track child processes.

[ghost]

Please please consider adding a command-line switch to enable this behavior for users who need it!

The non-solution I went with is basically adding a new flag called "noremove", which prevents the socket path from being unlinked when the socket is closed.

Thank you!