The $runstatedir is not being created at runtime

[alejandro-colomar]

See the following configuration. I used a prefix of /opt/local/unit to make sure there were no previous existing directories or stuff that was left by a previous unit installation, and so that I can wipe it easily, but this could perfectly use system paths.

The issue is that the system is allowed (and will normally do so) to wipe /run (or /var/run) at [re]boot time. Since we use various files in /run (at least, the pid file and the control socket), the FHS says that we should use a directory within /run:

https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.html#runRuntimeVariableData

Programs may have a subdirectory of /run; this is encouraged for programs that use more than one run-time file.

alx@asus5775:~/src/nginx/unit$ ./configure --prefix=/opt/local/unit \
                                           --modules=/opt/local/unit/lib/unit/modules \
                                           --state=/opt/local/unit/var/lib/unit \
                                           --pid=/opt/local/unit/var/run/unit/unit.pid \
                                           --log=/opt/local/unit/var/log/unit/unit.log \
                                           --control=unix:/opt/local/unit/var/run/unit/control.unit.sock \
                               | tail -n 27
Unit configuration summary:

  bin directory: ............. "/opt/local/unit/bin"
  sbin directory: ............ "/opt/local/unit/sbin"
  lib directory: ............. "/opt/local/unit/lib"
  include directory: ......... "/opt/local/unit/include"
  man pages directory: ....... "/opt/local/unit/share/man"
  modules directory: ......... "/opt/local/unit/lib/unit/modules"
  state directory: ........... "/opt/local/unit/var/lib/unit"
  tmp directory: ............. "/opt/local/unit/tmp"

  pid file: .................. "/opt/local/unit/var/run/unit/unit.pid"
  log file: .................. "/opt/local/unit/var/log/unit/unit.log"

  control API socket: ........ "unix:/opt/local/unit/var/run/unit/control.unit.sock"

  non-privileged user: ....... "nobody"
  non-privileged group: ...... ""

  IPv6 support: .............. YES
  Unix domain sockets support: YES
  TLS support: ............... NO

  process isolation: ......... USER NS PID NET UTS CGROUP

  debug logging: ............. NO

alx@asus5775:~/src/nginx/unit$ make -j >/dev/null; echo $?
0
alx@asus5775:~/src/nginx/unit$ sudo make install
install -d /opt/local/unit/sbin
install -p build/unitd /opt/local/unit/sbin/
install -d /opt/local/unit/var/lib/unit
install -d /opt/local/unit/share/man/man8
install -p -m644 build/unitd.8 /opt/local/unit/share/man/man8/
alx@asus5775:~/src/nginx/unit$ sudo /opt/local/unit/sbin/unitd 
2022/08/05 20:12:22 [alert] 21613#21613 bind(6, unix:/opt/local/unit/var/run/unit/control.unit.sock.tmp) failed (2: No such file or directory)

unitd fails to create the socket (because it assumes the parent dir exists always), and exits.

unitd should create the parent directory previous to attempting to bind(2) the socket.

[ac000]

The following patch seems to do the trick, but it's late and I may have missed something... (may be missing some error checking...)

diff --git a/src/nxt_socket.c b/src/nxt_socket.c
index a8e0d514..9a82bd61 100644
--- a/src/nxt_socket.c
+++ b/src/nxt_socket.c
@@ -177,6 +177,23 @@ nxt_socket_bind(nxt_task_t *task, nxt_socket_t s, nxt_sockaddr_t *sa)
     nxt_debug(task, "bind(%d, %*s)", s, (size_t) sa->length,
               nxt_sockaddr_start(sa));
 
+#if NXT_HAVE_UNIX_DOMAIN
+    if (sa->u.sockaddr.sa_family == AF_UNIX) {
+        char  *ptr;
+        char  *socket_dir;
+
+        socket_dir = nxt_strdup(sa->u.sockaddr_un.sun_path);
+
+        ptr = strrchr(socket_dir, '/');
+        if (ptr != NULL) {
+            *ptr = '\0';
+            nxt_fs_mkdir_all((const u_char *)socket_dir, 0777);
+        }
+
+        nxt_free(socket_dir);
+    }
+#endif
+
     if (nxt_fast_path(bind(s, &sa->u.sockaddr, sa->socklen) == 0)) {
         return NXT_OK;
     }

Edited to check if ptr is NULL and free socket_dir.

[alejandro-colomar]

$runstatedir is where the control socket lives. It should be 755.

[alejandro-colomar]

BTW, I didn't yet discern what this function does exactly. I know it creates the control socket, and I know it doesn't create the listeners, but is it used for any other sockets?

[ac000]

$runstatedir is where the control socket lives. It should be 755.

$ ls -ld /tmp/unit/var/run/unit/
drwxr-xr-x 2 andrew andrew 40 Aug  6 04:24 /tmp/unit/var/run/unit/

Assuming that mkdir function follows mkdir(2) then mode is modified by the umask. We could stick an explicit chmod(2) in there, but I'd say it's better to let the admin decide (via umask).

[ac000]

BTW, I didn't yet discern what this function does exactly. I know it creates the control socket, and I know it doesn't create the listeners, but is it used for any other sockets?

My brief testing (including UDS listener) shows my code only being called for the control socket.

[alejandro-colomar]

$runstatedir is where the control socket lives. It should be 755.

$ ls -ld /tmp/unit/var/run/unit/
drwxr-xr-x 2 andrew andrew 40 Aug  6 04:24 /tmp/unit/var/run/unit/

Assuming that mkdir function follows mkdir(2) then mode is modified by the umask. We could stick an explicit chmod(2) in there, but I'd say it's better to let the admin decide (via umask).

That makes sense. I prefer umask (please test the resulting directory permissions just to check, but LGTM).

[alejandro-colomar]

BTW, I didn't yet discern what this function does exactly. I know it creates the control socket, and I know it doesn't create the listeners, but is it used for any other sockets?

My brief testing (including UDS listener) shows my code only being called for the control socket.

Yeah, that's what my testing shows. I'll assume there are no other hidden paths that jump to this one, unless someone proves otherwise.

[alejandro-colomar]

BTW, the pid file also lives in that directory. Does this fix that too? I don't know in which order unit tries to create the socket and the pid file.

[alejandro-colomar]

The following patch seems to do the trick, but it's late and I may have missed something... (may be missing some error checking...)

diff --git a/src/nxt_socket.c b/src/nxt_socket.c
index a8e0d514..9a82bd61 100644
--- a/src/nxt_socket.c
+++ b/src/nxt_socket.c
@@ -177,6 +177,23 @@ nxt_socket_bind(nxt_task_t *task, nxt_socket_t s, nxt_sockaddr_t *sa)
     nxt_debug(task, "bind(%d, %*s)", s, (size_t) sa->length,
               nxt_sockaddr_start(sa));
 
+#if NXT_HAVE_UNIX_DOMAIN
+    if (sa->u.sockaddr.sa_family == AF_UNIX) {
+        char  *ptr;
+        char  *socket_dir;
+
+        socket_dir = nxt_strdup(sa->u.sockaddr_un.sun_path);

Why nxt_strdup() and not nxt_str_dup()? With the latter, you don't need to free it.

Although, I don't know which memory pool we have available here, and its lifetime. Freeing it immediately will make sure that we don't have an unnecessary string lying around for the whole lifetime of the program.

+
+        ptr = strrchr(socket_dir, '/');
+        if (ptr != NULL) {

Now I'm curious about what happens if the socket path is a relative filename with no / (and I hope no-one tries to use an abstract socket for the control socket).

+            *ptr = '\0';
+            nxt_fs_mkdir_all((const u_char *)socket_dir, 0777);
+        }
+
+        nxt_free(socket_dir);
+    }
+#endif
+
     if (nxt_fast_path(bind(s, &sa->u.sockaddr, sa->socklen) == 0)) {
         return NXT_OK;
     }

Edited to check if ptr is NULL and free socket_dir.

[ac000]

Ooh, I forgot about the pid file, good point.

While the pid file is created OK, we should perhaps not count on it and maybe the pid is being created some place else.

I wonder if we should do this for both the pid file and the control socket?

Here's the order that things currently happen

$ strace -f -e mkdir,mkdirat,chmod,fchmod,fchmodat,rename,renameat,unlink,unlinkat,open,openat,openat2 /tmp/unit/sbin/unitd --no-daemon
...
2022/08/06 15:45:57 [warn] 13647#13647 Unit is running unprivileged, then it cannot use arbitrary user and group.
mkdir("/tmp/unit/state/certs/", 0700)   = -1 EEXIST (File exists)
unlink("/tmp/unit/var/run/unit/control.unit.sock.tmp") = -1 ENOENT (No such file or directory)
mkdir("/tmp", 0777)                     = -1 EEXIST (File exists)
mkdir("/tmp/unit", 0777)                = -1 EEXIST (File exists)
mkdir("/tmp/unit/var", 0777)            = -1 EEXIST (File exists)
mkdir("/tmp/unit/var/run", 0777)        = -1 EEXIST (File exists)
mkdir("/tmp/unit/var/run/unit", 0777)   = -1 EEXIST (File exists)
chmod("/tmp/unit/var/run/unit/control.unit.sock.tmp", 0600) = 0
rename("/tmp/unit/var/run/unit/control.unit.sock.tmp", "/tmp/unit/var/run/unit/control.unit.sock") = 0
2022/08/06 15:45:57 [info] 13647#13647 unit 1.28.0 started
openat(AT_FDCWD, "/tmp/unit/unit.log", O_WRONLY|O_CREAT|O_APPEND|O_NONBLOCK, 0600) = 7
openat(AT_FDCWD, "/tmp/unit/var/run/unit/unit.pid", O_WRONLY|O_CREAT|O_TRUNC|O_NONBLOCK, 0644) = 8
strace: Process 13648 attached
...

(Edited to show strace output)

[ac000]

+        socket_dir = nxt_strdup(sa->u.sockaddr_un.sun_path);

Why nxt_strdup() and not nxt_str_dup()? With the latter, you don't need to free it.

Probably because I wasn't really aware of it ;-)

So it's using a memory pool, OK, but we only really need that string temporarily, but it's not a lot of memory, so I'm not overly fussed...

Although, I don't know which memory pool we have available here, and its lifetime. Freeing it immediately will make sure that we don't have an unnecessary string lying around for the whole lifetime of the program.

Right.

+
+        ptr = strrchr(socket_dir, '/');
+        if (ptr != NULL) {

Now I'm curious about what happens if the socket path is a relative filename with no / (and I hope no-one tries to use an abstract socket for the control socket).

I thought I'd better handle that case (have user ... ), I assume it will just try to create the socket in the processes current working directory.

[ac000]

Assuming that mkdir function follows mkdir(2) then mode is modified by the umask. We could stick an explicit chmod(2) in there, but I'd say it's better to let the admin decide (via umask).

That makes sense. I prefer umask (please test the resulting directory permissions just to check, but LGTM).

With a umask of 0022

$ ls -ld /tmp/unit/var{,/run,/run/unit}
drwxr-xr-x 3 andrew andrew 60 Aug  7 00:44 /tmp/unit/var
drwxr-xr-x 3 andrew andrew 60 Aug  7 00:44 /tmp/unit/var/run
drwxr-xr-x 2 andrew andrew 80 Aug  7 00:44 /tmp/unit/var/run/unit

with a umask of 0002

$ ls -ld /tmp/unit/var{,/run,/run/unit}
drwxrwxr-x 3 andrew andrew 60 Aug  7 00:41 /tmp/unit/var
drwxrwxr-x 3 andrew andrew 60 Aug  7 00:41 /tmp/unit/var/run
drwxrwxr-x 2 andrew andrew 80 Aug  7 00:41 /tmp/unit/var/run/unit

Or did you mean to use access(2) or some such in the code?

[alejandro-colomar]

No, I think the code is fine as proposed in your patch. I just wanted to make sure that the obvious expected behavior was indeed occuring, which it is (per your ls(1) runs). So LGTM. :)

[alejandro-colomar]

Fixed by 57fc920