forbid relative paths with daemonization

Relative paths are incompatible with daemonization, as it
does not work for SIGUSR2 upgrades (since daemonize forces
the server to run in "/").  Relative paths are confusing
and error-prone anyways, so do not allow users to specify
them along with --daemonize.

4 files changed, 157 insertions, 1 deletions

diff --git a/cfg.c b/cfg.c
index edd7883..2b3430f 100644
--- a/cfg.c
+++ b/cfg.c
@@ -13,6 +13,7 @@ static void cfg_free_internal(struct mog_cfg *cfg)
 {
         mog_free(cfg->docroot);
         mog_free(cfg->pidfile);
+        mog_free(cfg->config);
         mog_free(cfg->configfile);
         mog_free(cfg->server);
         mog_addrinfo_free(&cfg->mgmtlisten);
@@ -63,6 +64,7 @@ struct mog_cfg * mog_cfg_new(const char *configfile)
         struct mog_cfg *cfg = xzalloc(sizeof(struct mog_cfg));
 
         cfg->configfile = mog_canonpath_die(configfile, CAN_EXISTING);
+        cfg->config = xstrdup(configfile);
 
         switch (hash_insert_if_absent(all_cfg, cfg, NULL)) {
         case 0:
@@ -111,6 +113,38 @@ static size_t nr_config(void)
         return all_cfg ? hash_get_n_entries(all_cfg) : 0;
 }
 
+#define RELPATH_ERR \
+"relative paths are incompatible with --daemonize and SIGUSR2 upgrades"
+static void validate_daemonize(struct mog_cfg *cli)
+{
+        size_t nerr = 0;
+        const char *path = getenv("PATH");
+        const char *p;
+
+        hash_do_for_each(all_cfg, mog_cfg_validate_daemon, &nerr);
+
+        /* cli may have merged identical settings */
+        if (!nerr)
+                mog_cfg_validate_daemon(cli, &nerr);
+
+        p = path;
+        while (*p) {
+                if (*p == '/') {
+                        p = strchr(p, ':');
+                        if (!p)
+                                break;
+                        p++;
+                        continue;
+                }
+                warn("PATH environment contains relative path: %s", p);
+                nerr++;
+                break;
+        }
+
+        if (nerr)
+                die(RELPATH_ERR);
+}
+
 #define MULTI_CFG_ERR \
 "--multi must be set if using multiple --config/-c switches"
 
@@ -132,6 +166,8 @@ void mog_cfg_validate_or_die(struct mog_cfg *cli)
                 if (!mog_cfg_multi)
                         die(MULTI_CFG_ERR);
         }
+        if (cli->daemonize)
+                validate_daemonize(cli);
         mog_set_maxconns(cli->maxconns);
 }
 
diff --git a/cfg.h b/cfg.h
index bf65d29..78c7312 100644
--- a/cfg.h
+++ b/cfg.h
@@ -8,7 +8,8 @@ struct mog_cfg {
         bool daemonize;
         unsigned long maxconns;
         const char *pidfile;
-        const char *configfile;
+        const char *configfile; /* expanded path */
+        const char *config; /* command-line arg */
         const char *server;
         struct mog_addrinfo *httplisten;
         struct mog_addrinfo *mgmtlisten;
@@ -19,6 +20,7 @@ struct mog_cfg {
 void mog_cfg_validate_or_die(struct mog_cfg *cli);
 bool mog_cfg_validate_one(void *ent, void *cli);
 bool mog_cfg_validate_multi(void *ent, void *cli);
+bool mog_cfg_validate_daemon(void *ent, void *nerr);
 void mog_cfg_die_if_cli_set(struct mog_cfg *);
 void mog_cfg_merge_defaults(struct mog_cfg *);
 void mog_cfg_check_server(struct mog_cfg *);
diff --git a/cfg_validate.c b/cfg_validate.c
index d07b96c..65001e7 100644
--- a/cfg_validate.c
+++ b/cfg_validate.c
@@ -85,6 +85,26 @@ bool mog_cfg_validate_multi(void *ent_ptr, void *cli_ptr)
         return true;
 }
 
+static void warn_daemonize(size_t *nerr, const char *key, const char *val)
+{
+        warn("%s=%s must use an absolute path", key, val);
+        (*nerr)++;
+}
+
+bool mog_cfg_validate_daemon(void *ent_ptr, void *nerr)
+{
+        struct mog_cfg *ent = ent_ptr;
+
+        if (ent->config&& ent->config[0] != '/')
+                warn_daemonize(nerr, "config", ent->config);
+        if (ent->pidfile && ent->pidfile[0] != '/')
+                warn_daemonize(nerr, "pidfile", ent->pidfile);
+        if (ent->docroot && ent->docroot[0] != '/')
+                warn_daemonize(nerr, "docroot", ent->docroot);
+
+        return true;
+}
+
 static void die_if_set(const void *a, const char *sw)
 {
         if (!a) return;
diff --git a/test/cmogstored-cfg.rb b/test/cmogstored-cfg.rb
index ba413a2..8086ac3 100644
--- a/test/cmogstored-cfg.rb
+++ b/test/cmogstored-cfg.rb
@@ -3,6 +3,7 @@
 require 'test/test_helper'
 require 'stringio'
 require 'net/http'
+require 'timeout'
 
 class TestCmogstoredConfig < Test::Unit::TestCase
   def setup
@@ -41,6 +42,17 @@ class TestCmogstoredConfig < Test::Unit::TestCase
     pids.split(/\s+/).grep(/\A\d+\z/).map { |x| x.to_i }.sort
   end
 
+  def expand_suppressions!(cmd)
+    cmd.map! do |x|
+      case x
+      when /\A--suppressions=(\S+)\z/
+        "--suppressions=#{File.expand_path($1)}"
+      else
+        x
+      end
+    end
+  end
+
   def test_worker_processes
     nproc = 2
     @cmd << "--worker-processes=#{nproc}"
@@ -394,4 +406,90 @@ class TestCmogstoredConfig < Test::Unit::TestCase
       assert_raises(Errno::ESRCH) { Process.kill(0, pid) }
     end
   end
+
+  def test_PATH_env_has_relpath
+    @cmd << "--docroot=#@tmpdir"
+    @cmd << "--daemonize"
+    @cmd << "--mgmtlisten=#@host:#@port"
+    tmp = Tempfile.new("err")
+    @pid = fork do
+      ENV["PATH"] = "#{ENV["PATH"]}:."
+      $stderr.reopen(tmp)
+      exec(*@cmd)
+    end
+    _, status = Process.waitpid2(@pid)
+    assert ! status.success?, status.inspect
+    tmp.rewind
+    lines = tmp.read
+    assert_match(/PATH environment contains relative path/, lines)
+    assert_match(/relative paths are incompatible with --daemonize/, lines)
+  end
+
+  def test_docroot_has_relpath
+    @cmd << "--daemonize"
+    @cmd << "--mgmtlisten=#@host:#@port"
+    tmp = Tempfile.new("err")
+    dirname, basename = File.split(@tmpdir)
+    @cmd << "--docroot=#{basename}"
+    @pid = fork do
+      expand_suppressions!(@cmd)
+      Dir.chdir(dirname)
+      $stderr.reopen(tmp)
+      exec(*@cmd)
+    end
+    _, status = Process.waitpid2(@pid)
+    assert ! status.success?, status.inspect
+    tmp.rewind
+    lines = tmp.read
+    assert_match(/docroot=#{basename} must use an absolute/, lines)
+    assert_match(/relative paths are incompatible with --daemonize/, lines)
+  end
+
+  def test_config_is_relpath
+    cfg = Tempfile.new("cfg")
+    cfg.puts "mgmtlisten = #@host:#@port"
+    cfg.puts "docroot = #@tmpdir"
+    cfg.puts "daemonize"
+    cfg.flush
+    dirname, basename = File.split(cfg.path)
+    @cmd << "--config=#{basename}"
+    tmp = Tempfile.new("err")
+    @pid = fork do
+      expand_suppressions!(@cmd)
+      Dir.chdir(dirname)
+      $stderr.reopen(tmp)
+      exec(*@cmd)
+    end
+    _, status = Process.waitpid2(@pid)
+    assert ! status.success?, status.inspect
+    tmp.rewind
+    lines = tmp.read
+    assert_match(/config=#{basename} must use an absolute/, lines)
+    assert_match(/relative paths are incompatible with --daemonize/, lines)
+  end
+
+  def test_config_has_pidfile_relpath
+    pid = Tempfile.new("pid")
+    dirname, basename = File.split(pid.path)
+    cfg = Tempfile.new("cfg")
+    cfg.puts "mgmtlisten = #@host:#@port"
+    cfg.puts "docroot = #@tmpdir"
+    cfg.puts "pidfile = #{basename}"
+    cfg.puts "daemonize"
+    cfg.flush
+    @cmd << "--config=#{cfg.path}"
+    tmp = Tempfile.new("err")
+    @pid = fork do
+      expand_suppressions!(@cmd)
+      Dir.chdir(dirname)
+      $stderr.reopen(tmp)
+      exec(*@cmd)
+    end
+    _, status = Process.waitpid2(@pid)
+    assert ! status.success?, status.inspect
+    tmp.rewind
+    lines = tmp.read
+    assert_match(/pidfile=#{basename} must use an absolute/, lines)
+    assert_match(/relative paths are incompatible with --daemonize/, lines)
+  end
 end