Add flag to prevent unlinking of socket file

This is something I tried to properly fix for ages, but I haven't found
a very good solution. The commit is not what I'd call a good solution or
even a real solution at all, but it should work around problematic
cases.

So here is (simplified) how ip2unix currently tracks socket file
descriptors:

  * We have an internal registry, which maps the file descriptors of the
    current process to our internal Socket state.
  * Whenever we get a new socket() call, we insert the file descriptor
    into the registry. Note that at this point, we can't make a final
    decision about how to handle this socket, because we only know the
    socket family (eg. AF_INET or AF_INET6) but no details about ports
    or IP addresses.
  * Once we get a bind() or connect() call, we look up the file
    descriptor in the registry and see whether we have a matching rule.
  * If the rule matches, we go ahead and execute the corresponding
    action (eg. convert to Unix socket, blackhole, reject, etc...), if
    it doesn't we just remove the file descriptor from the registry and
    call the real bind() or connect() function.
  * Once the socket is closed, we remove the file descriptor from the
    registry and (where applicable) unlink the socket file.

This all works fine if everything is run in order and without any
multiprocessing involved, but as soon as the application invokes
clone(), things start to get ugly.

Essentially we have two clone() flags that are problematic:

  CLONE_VM:

    If set, the calling process and the child process run in the same
    memory space. In particular, memory writes performed by the calling
    process or by the child process are also visible in the other
    process.

    If not set, the child process runs in a separate copy of the memory
    space of the calling process at the time of the clone call. Memory
    writes or file mappings/unmappings performed by one of the processes
    do not affect the other.

  CLONE_FILES:

    If set, the calling process and the child process share the same
    file descriptor table. Any file descriptor created by the calling
    process or by the child process is also valid in the other process.

    If not set, the child process inherits a copy of all file
    descriptors opened in the calling process at the time of the clone
    call. Subsequent operations that open or close file descriptors, or
    change file descriptor flags, performed by either the calling
    process or the child process do not affect the other process.

Here is how these flags are affecting us:

 Both CLONE_VM and CLONE_FILES set:

   Processes share memory and also share file descriptors, which
   essentially means that we don't need to do anything, because the
   registry and the file descriptor table is all that we need for
   properly tracking the state.

 Only CLONE_VM set:

   This is a little more tricky since we have one shared registry across
   both processes, but the file descriptor tables are copied. This means
   that whenever we're closing a socket in the second process, we can
   not yet unlink the socket file until the file descriptor is also
   closed in the first process. We could basically just implement a
   garbage collector when the used socket file descriptors reach zero.

 Only CLONE_FILES set:

   Here we have two registries, but one shared file descriptor table. In
   this scenario, we could use a dedicated file descriptor that we can
   use for sharing state between two registries.

 None of those flags set:

   Again, two registries, but also a copy of the file descriptor table.

   This is our nightmare scenario, consider this chain of events:

     * Process 1 creates a socket
     * Process 1 clone()s into process 2
     * Process 2 closes the socket and ip2unix unlinks the file
     * ... wait, what!?

   Process 1 still has to be able to operate on the socked but we
   essentially made the socket a blackhole since it's no longer
   reachable via the filesystem. There is also no way to handle this
   properly without also introducing some side channel that tracks the
   state from outside.

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.

This doesn't fully solve the issue because ip2unix still removes the
socket file descriptor from the registry and also has no way to even
know if it is a socket it should track.

However, this allows us to deal with the issue until we have found a
real solution and the test case I added is essentially replicating most
programs I found in the wild.

Signed-off-by: aszlig <aszlig@nix.build>
Closes: #16

Author: aszlig

CHANGELOG.md

@@ -17,6 +17,7 @@ The format is based on [Keep a Changelog], and this project adheres to
 - Support for Linux abstract sockets.
 - Support for matching an existing Unix domain socket or abstract socket.
 - Add `stream`/`datagram` aliases for `tcp`/`udp` socket types.
+- Add flag to prevent unlinking of socket files when closing sockets.
 
 ### Changed
 - Rule files (`-f`) are now just a list of newline-separated rule (`-r`)

README.adoc

@@ -282,6 +282,15 @@ Placeholders are allowed here and are substituted accordingly:
                     datagram socket)
 *%%*;; verbatim `%`
 
+*noremove*::
+If this flag is given in conjunction with a <<rule-socket-path,*path*>>, the
+socket file is not removed when the socket is closed.
++
+This works around an issue with more complex programs that spawn subprocesses
+or threads without sharing memory or cloning the file descriptor table. In some
+scenarios *ip2unix* might be unable to correctly track sockets and might
+accidentally remove the socket file too early.
+
 ifndef::without-abstract[]
 *abstract*='NAME'::
 An abstract namespace Unix domain socket not being created in the file system

src/rules/parse.cc

@@ -221,6 +221,8 @@ static std::optional<Rule> parse_rule(const std::string &file, int pos,
 {
     Rule rule;
 
+    bool noremove = false;
+
     for (const auto &foo : doc) {
         std::string key = foo.first.as<std::string>();
         YAML::Node value = foo.second;
@@ -293,6 +295,8 @@ static std::optional<Rule> parse_rule(const std::string &file, int pos,
             RULE_CONVERT(rule.action.blackhole, "blackhole", bool, "bool");
         } else if (key == "ignore") {
             RULE_CONVERT(rule.action.ignore, "ignore", bool, "bool");
+        } else if (key == "noRemove") {
+            RULE_CONVERT(noremove, "noRemove", bool, "bool");
         } else if (key == "socketPath") {
             if (rule.action.socket_path) {
                 RULE_ERROR("Socket path or abstract name already specified.");
@@ -325,6 +329,19 @@ static std::optional<Rule> parse_rule(const std::string &file, int pos,
         }
     }
 
+    if (noremove) {
+        if (
+            rule.action.socket_path &&
+            rule.action.socket_path->is_real_file()
+        ) {
+            rule.action.socket_path->unlink = false;
+        } else {
+            RULE_ERROR("The noremove flag can only be used for Unix socket"
+                       " paths.");
+            return std::nullopt;
+        }
+    }
+
     std::optional<std::string> errmsg = validate_rule(rule);
     if (errmsg) {
         RULE_ERROR(errmsg.value());
@@ -414,6 +431,7 @@ std::optional<Rule> parse_rule_arg(size_t rulepos, const std::string &arg)
 
     size_t errpos = 0, valpos = 0;
     size_t errlen = 0;
+    bool noremove = false;
 
     for (size_t i = 0, arglen = arg.length(); i <= arglen; ++i) {
         if (key) {
@@ -532,6 +550,8 @@ std::optional<Rule> parse_rule_arg(size_t rulepos, const std::string &arg)
                 rule.action.blackhole = true;
             } else if (buf == "ignore") {
                 rule.action.ignore = true;
+            } else if (buf == "noremove") {
+                noremove = true;
             } else {
                 print_arg_error(rulepos, arg, errpos, errlen, "unknown flag");
                 return std::nullopt;
@@ -552,6 +572,21 @@ std::optional<Rule> parse_rule_arg(size_t rulepos, const std::string &arg)
         buf += arg[i];
     }
 
+    if (noremove) {
+        if (
+            rule.action.socket_path &&
+            rule.action.socket_path->is_real_file()
+        ) {
+            rule.action.socket_path->unlink = false;
+        } else {
+            print_arg_error(
+                rulepos, arg, 0, 0,
+                "The noremove flag can only be used for Unix socket paths."
+            );
+            return std::nullopt;
+        }
+    }
+
     std::optional<std::string> errmsg = validate_rule(rule);
     if (errmsg) {
         print_arg_error(rulepos, arg, 0, 0, errmsg.value());
@@ -662,9 +697,13 @@ void print_rules(std::vector<Rule> &rules, std::ostream &out)
                         << std::endl;
                 } else {
 #endif
-                    out << "  Socket path: "
-                        << rule.action.socket_path->value
-                        << std::endl;
+                    out << "  Socket path: " << rule.action.socket_path->value;
+                    if (rule.action.socket_path->unlink) {
+                        out << " (will be removed on close)";
+                    } else {
+                        out << " (will not be removed on close)";
+                    }
+                    out << std::endl;
 #if defined(__linux__)
                 }
 #endif

src/serial.cc

@@ -170,6 +170,7 @@ void serialise(const SocketPath &path, std::ostream &out)
     }
 
     serialise(path.value, out);
+    serialise(path.unlink, out);
 }
 
 MaybeError deserialise(std::istream &in, SocketPath *out)
@@ -195,7 +196,9 @@ MaybeError deserialise(std::istream &in, SocketPath *out)
                  + c + "' used as socket path type.";
     }
 
-    return deserialise(in, &out->value);
+    MaybeError err;
+    if ((err = deserialise(in, &out->value))) return err;
+    return deserialise(in, &out->unlink);
 }
 
 void serialise(const Rule &rule, std::ostream &out)

src/socket.cc

@@ -203,7 +203,7 @@ SocketPath Socket::format_sockpath(const SocketPath &path,
         out += path.value[i];
     }
 
-    return SocketPath(path.type, out);
+    return SocketPath(path.type, out, path.unlink);
 }
 
 /*
@@ -371,8 +371,12 @@ int Socket::bind(const SockAddr &addr, const SocketPath &path)
         ret = real::bind(this->fd, dest.cast(), dest.size());
         if (ret == 0) {
             Socket::sockpath_registry.insert(newpath);
-            if (newpath.is_real_file())
+            if (newpath.is_real_file() && newpath.unlink) {
+                LOG(DEBUG) << "Marking socket file '" << newpath.value
+                           << "' for deletion when closing fd "
+                           << this->fd << '.';
                 this->unlink_sockpath = newpath.value;
+            }
         }
     }
 
@@ -680,13 +684,12 @@ int Socket::close(void)
         }
     }
 
-    Socket::registry.erase(this->fd);
-    LOG(INFO) << "Socket fd " << this->fd << " unregistered.";
+    this->unregister();
     return ret;
 }
 
 void Socket::unregister(void)
 {
-    LOG(DEBUG) << "Unregistering socket fd " << this->fd << '.';
     Socket::registry.erase(this->fd);
+    LOG(INFO) << "Socket fd " << this->fd << " unregistered.";
 }

src/socketpath.hh

@@ -7,8 +7,12 @@
 struct SocketPath {
     enum class Type { ABSTRACT, FILESYSTEM };
 
-    inline SocketPath() : type(Type::FILESYSTEM), value() {}
-    inline SocketPath(Type t, const std::string &v) : type(t), value(v) {}
+    inline SocketPath()
+        : type(Type::FILESYSTEM), value(), unlink(true) {}
+    inline SocketPath(Type t, const std::string &v)
+        : type(t), value(v), unlink(true) {}
+    inline SocketPath(Type t, const std::string &v, bool ul)
+        : type(t), value(v), unlink(ul) {}
 
     inline bool operator==(const SocketPath &other) const {
         return this->type == other.type && this->value == other.value;
@@ -24,6 +28,7 @@ struct SocketPath {
 
     Type type;
     std::string value;
+    bool unlink;
 };
 
 namespace std {

tests/test_preliminary_unlink.py

@@ -0,0 +1,47 @@
+import sys
+import subprocess
+import socket
+
+from helper import IP2UNIX
+
+TESTPROG = r'''
+import os
+import sys
+import socket
+
+sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+sock.bind(('1.2.3.4', 1234))
+sock.listen(10)
+
+childpid = os.fork()
+if childpid == 0:
+    os.closerange(3, 65536)
+    raise SystemExit
+
+assert os.WEXITSTATUS(os.waitpid(childpid, 0)[1]) == 0
+
+sys.stdout.write('ready\n')
+sys.stdout.flush()
+
+with sock.accept()[0] as conn:
+    assert conn.recv(3) == b'foo'
+'''
+
+
+def test_preliminary_unlink(tmpdir):
+    """
+    Regression test for https://github.com/nixcloud/ip2unix/issues/16
+    """
+    sockfile = str(tmpdir.join('test.sock'))
+
+    cmd = [
+        IP2UNIX, '-r', 'port=1234,addr=1.2.3.4,noremove,path=' + sockfile,
+        sys.executable, '-c', TESTPROG,
+    ]
+
+    with subprocess.Popen(cmd, stdout=subprocess.PIPE) as client:
+        assert client.stdout.readline() == b'ready\n'
+        with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock:
+            sock.connect(sockfile)
+            sock.sendall(b'foo')
+        assert client.wait() == 0

tests/test_program_args.py

@@ -45,10 +45,10 @@ def test_rule_longopts(tmpdir):
         stdout = subprocess.check_output(deprecated_cmd,
                                          stderr=subprocess.STDOUT)
         assert b"is deprecated" in stdout
-        assert b"path: /test\n" in stdout
+        assert b"path: /test (will" in stdout
 
     stdout = subprocess.check_output([IP2UNIX, '-cp', '--rule', 'path=/test'])
-    assert b"path: /test\n" in stdout
+    assert b"path: /test (will" in stdout
 
 
 def test_no_program():

tests/test_rule_args.py

@@ -42,7 +42,7 @@ def test_path_subst(self):
         }
         for val, expect in fixtures.items():
             stdout, stderr = self.check_rules("path=" + val)
-            self.assertIn("Socket path: " + expect + "\n", stdout)
+            self.assertIn("Socket path: " + expect + " (will", stdout)
 
     def test_good(self):
         fixtures = {
@@ -54,9 +54,9 @@ def test_good(self):
             "path=/eee\\\\,out": "Direction: outgoing\n",
             "path=/fff\\\\,tcp,udp": "IP Type: UDP\n",
             "path=/ggg\\\\,in,out": "Direction: outgoing\n",
-            "path=/hhh\\\\": "Socket path: /hhh\\\n",
+            "path=/hhh\\\\": "Socket path: /hhh\\ (will",
             "path=/iii\\,,out": "IP Type: TCP and UDP\n",
-            "path=/jjj\\,,in": "Socket path: /jjj,\n",
+            "path=/jjj\\,,in": "Socket path: /jjj, (will",
             "path=/kkk\\\\,stream": "IP Type: TCP\n",
             "path=/lll\\\\,datagram": "IP Type: UDP\n",
             "path=/mmm\\\\,dgram": "IP Type: UDP\n",
@@ -65,8 +65,9 @@ def test_good(self):
             "reject=999999": "calls with errno <unknown>.\n",
             "in,blackhole": "Blackhole the socket.\n",
             "in,ignore": "Don't handle this socket.\n",
-            "path=foo": "Socket path: " + os.getcwd() + "/foo\n",
+            "path=foo": "Socket path: " + os.getcwd() + "/foo (will",
             "from-unix=xyz,path=/foo": "domain socket path matching: xyz\n",
+            "path=bar,noremove": "will not be removed on close",
         }
         for val, expect in fixtures.items():
             stdout, stderr = self.check_rules(val)

tests/test_rule_file.py

@@ -193,6 +193,22 @@ def test_ignore_with_reject(self):
     def test_ignore_with_blackhole(self):
         self.assert_bad_rules([{'blackhole': True, 'ignore': True}])
 
+    def test_noremove_without_sockpath(self):
+        self.assert_bad_rules([{'reject': True, 'noRemove': True}])
+        self.assert_bad_rules([{'blackhole': True, 'noRemove': True}])
+        self.assert_bad_rules([{'ignore': True, 'noRemove': True}])
+
+    @systemd_only
+    def test_noremove_with_systemd(self):
+        self.assert_bad_rules([{'socketActivation': True, 'noRemove': True}])
+
+    @abstract_sockets_only
+    def test_noremove_with_abstract(self):
+        self.assert_bad_rules([{'abstract': 'foo', 'noRemove': True}])
+
+    def test_noremove_with_sockpath(self):
+        self.assert_good_rules([{'socketPath': '/foo', 'noRemove': True}])
+
     @systemd_only
     def test_ignore_with_systemd(self):
         self.assert_bad_rules([{'socketActivation': True, 'ignore': True}])
@@ -274,17 +290,17 @@ def test_new_style_rule_files(self):
             b'  IP Type: TCP and UDP\n'
             b'  Address: <any>\n'
             b'  Port: 1234\n'
-            b'  Socket path: /foo\n'
+            b'  Socket path: /foo (will be removed on close)\n'
             b'Rule #2:\n'
             b'  Direction: incoming\n'
             b'  IP Type: TCP and UDP\n'
             b'  Address: 9.8.7.6\n'
             b'  Port: <any>\n'
-            b'  Socket path: /bar\n'
+            b'  Socket path: /bar (will be removed on close)\n'
             b'Rule #3:\n'
             b'  Direction: outgoing\n'
             b'  IP Type: TCP and UDP\n'
             b'  Address: <any>\n'
             b'  Port: 4321\n'
-            b'  Socket path: /foobar\n'
+            b'  Socket path: /foobar (will be removed on close)\n'
         )