From ba8f674e15abd9862565d2d43e88ef29a7eac00f Mon Sep 17 00:00:00 2001
From: "Nicholas J. Kain" <nicholas@kain.us>
Date: Fri, 25 Feb 2022 04:53:02 -0500
Subject: [PATCH] nk/exec: Simplify and remove double null termination.

The current code pads with an extra character that is then rewritten
into a null character.  This isn't necessary with post-C99
implementations of standardized snprintf, so get rid of it.

Also add a note warning that nk_generate_env() and nk_execute()
are not async signal safe and are thus unsuitable for use in
multithreaded processes.

nk_execute() could be rewritten to be async signal safe without much
trouble, as the only problem point is snprintf() which is not guaranteed
to be async signal safe by POSIX.

However, nk_generate_env() performs chroot() if a chroot_path is
specified, and chroot() is not async signal safe in POSIX.
Additionally, malloc() can be called in rare cases where user
information fields are very long, and malloc() is obviously not async
signal safe.  Finally, snprintf() is used here, too, but it could be
replaced.

Converting to posix_spawn() is a no-go because posix_spawn() has no
facility for changing rlimits or chroot on the spawned process.

In summary, I don't think the gains are worth it.  Multithreaded
processes should just not fork().
---
 nk/exec.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/nk/exec.c b/nk/exec.c
index 7341a6e..108a2c0 100644
--- a/nk/exec.c
+++ b/nk/exec.c
@@ -12,6 +12,17 @@
 #include "nk/exec.h"
 #include "nk/io.h"
 
+/*
+ * Note that neither nk_generate_env or nk_execute are async signal safe, so
+ * these functions should only be called after fork() in a non-multithreaded
+ * process.
+ *
+ * I don't consider this to be a problem in general, since in a multithreaded process
+ * it would be far better to fork off a subprocess early on before threads are
+ * created and use a socketpair() to request subprocess creation from the
+ * single-threaded subprocess from the multithreaded main program.
+ */
+
 #define DEFAULT_ROOT_PATH "/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin"
 #define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
 #define MAX_ARGS 256
@@ -19,9 +30,8 @@
 
 #define NK_GEN_ENV(GEN_STR, ...) do { \
         if (env_offset >= envlen) return -3; \
-        ssize_t snlen = snprintf(envbuf, envbuflen, GEN_STR "0", __VA_ARGS__); \
+        ssize_t snlen = snprintf(envbuf, envbuflen, GEN_STR, __VA_ARGS__); \
         if (snlen < 0 || (size_t)snlen >= envbuflen) return -2; \
-        if (snlen > 0) envbuf[snlen-1] = 0; \
         env[env_offset++] = envbuf; envbuf += snlen; envbuflen -= (size_t)snlen; \
     } while (0)
 
@@ -108,14 +118,13 @@ out:
 }
 
 #define NK_GEN_ARG(GEN_STR, ...) do { \
-        ssize_t snlen = snprintf(argbuf, argbuflen, GEN_STR "0", __VA_ARGS__); \
+        ssize_t snlen = snprintf(argbuf, argbuflen, GEN_STR, __VA_ARGS__); \
         if (snlen < 0 || (size_t)snlen >= argbuflen) { \
             static const char errstr[] = "nk_execute: constructing argument list failed\n"; \
             safe_write(STDERR_FILENO, errstr, sizeof errstr); \
             _Exit(EXIT_FAILURE); \
         } \
-        if (snlen > 0) argbuf[snlen-1] = 0; \
-        argv[curv] = argbuf; argv[++curv] = (char *)0; \
+        argv[curv] = argbuf; argv[++curv] = NULL; \
         argbuf += snlen; argbuflen -= (size_t)snlen; \
     } while (0)
 
@@ -167,8 +176,7 @@ endarg:
                     safe_write(STDERR_FILENO, errstr, sizeof errstr);
                     _Exit(EXIT_FAILURE);
                 }
-                const size_t len = (size_t)(p - q);
-                NK_GEN_ARG("%.*s", (int)len, q);
+                NK_GEN_ARG("%.*s", (int)(p - q), q);
                 q = p + 1;
                 if (atend || curv >= (MAX_ARGS - 1))
                     break;