[PATCH 0/3] qemu/qsd: Unlink absolute PID file path

[PATCH 0/3] qemu/qsd: Unlink absolute PID file path

[Hanna Reitz]

Hi,

QEMU (the system emulator) and the storage daemon (QSD) write their PID
to the given file when you specify --pidfile.  They keep the path around
and register exit handlers (QEMU uses an exit notifier, QSD an atexit()
function) to unlink this file when the process terminates.  These
handlers unlink precisely the path that the user has specified via
--pidfile, so if it was a relative path and the process has at any point
changed its working directory, the path no longer points to the PID
file, and so the unlink() will fail (or worse).

When using --daemonize, the process will always change its working
directory to /, so this problem basically always appears when using
--daemonize and --pidfile in conjunction.

(It gets even worse with QEMU’s --chroot, but I’m not sure whether
there’s any trivial fix for that (whether chroot is used or not is
confined to os-posix.c, so this would need to be externally visible; and
I guess the plain would be to skip the unlink() in that case?), so I’d
rather just skip that for now... :/)

We can fix the problem by running realpath() once the PID file has been
created, so we get an absolute path that we can unlink in the exit
handler.  This is done here.

(Another way to fix this might be to open the directory the PID file is
in, keep the FD around, and use unlinkat() in the exit handler.  I
couldn’t see any real benefit for this, though, so I didn’t go that
route.  It might be beneficial for the --chroot case, but then again,
precisely in that case we probably don’t want to keep random directory
FDs around.)


Hanna Reitz (3):
  qsd: Unlink absolute PID file path
  vl: Conditionally register PID file unlink notifier
  vl: Unlink absolute PID file path

 softmmu/vl.c                         | 42 +++++++++++++++++++++-------
 storage-daemon/qemu-storage-daemon.c | 11 +++++++-
 2 files changed, 42 insertions(+), 11 deletions(-)

-- 
2.35.3

[PATCH 1/3] qsd: Unlink absolute PID file path

[Hanna Reitz]

After writing the PID file, we register an atexit() handler to unlink it
when the process terminates.  However, if the process has changed its
working directory in the meantime (e.g. in os_setup_post() when
daemonizing), this will not work when the PID file path was relative.
Therefore, pass the absolute path (created with realpath()) to the
unlink() call in the atexit() handler.

(realpath() needs a path pointing to an existing file, so we cannot use
it before qemu_write_pidfile().)

Reproducer:
$ cd /tmp
$ qemu-storage-daemon --daemonize --pidfile qsd.pid
$ file qsd.pid
qsd.pid: ASCII text
$ kill $(cat qsd.pid)
$ file qsd.pid
qsd.pid: ASCII text

(qsd.pid should be gone after the process has terminated.)

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2092322
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index c104817cdd..7b8d6cf381 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -61,6 +61,7 @@
 #include "trace/control.h"
 
 static const char *pid_file;
+static char *pid_file_realpath;
 static volatile bool exit_requested = false;
 
 void qemu_system_killed(int signal, pid_t pid)
@@ -349,7 +350,7 @@ static void process_options(int argc, char *argv[], bool pre_init_pass)
 
 static void pid_file_cleanup(void)
 {
-    unlink(pid_file);
+    unlink(pid_file_realpath);
 }
 
 static void pid_file_init(void)
@@ -365,6 +366,14 @@ static void pid_file_init(void)
         exit(EXIT_FAILURE);
     }
 
+    pid_file_realpath = g_malloc(PATH_MAX);
+    if (!realpath(pid_file, pid_file_realpath)) {
+        error_report("cannot resolve PID file path: %s: %s",
+                     pid_file, strerror(errno));
+        unlink(pid_file);
+        exit(EXIT_FAILURE);
+    }
+
     atexit(pid_file_cleanup);
 }
 
-- 
2.35.3

Re: [PATCH 1/3] qsd: Unlink absolute PID file path

[Daniel P. Berrangé]

On Thu, Jun 09, 2022 at 02:26:59PM +0200, Hanna Reitz wrote:
> After writing the PID file, we register an atexit() handler to unlink it
> when the process terminates.  However, if the process has changed its
> working directory in the meantime (e.g. in os_setup_post() when
> daemonizing), this will not work when the PID file path was relative.
> Therefore, pass the absolute path (created with realpath()) to the
> unlink() call in the atexit() handler.
> 
> (realpath() needs a path pointing to an existing file, so we cannot use
> it before qemu_write_pidfile().)
> 
> Reproducer:
> $ cd /tmp
> $ qemu-storage-daemon --daemonize --pidfile qsd.pid
> $ file qsd.pid
> qsd.pid: ASCII text
> $ kill $(cat qsd.pid)
> $ file qsd.pid
> qsd.pid: ASCII text
> 
> (qsd.pid should be gone after the process has terminated.)
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2092322
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  storage-daemon/qemu-storage-daemon.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 2/3] vl: Conditionally register PID file unlink notifier

[Hanna Reitz]

Currently, the exit notifier for unlinking the PID file is registered
unconditionally.  Limit it to only when we actually do create a PID
file.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 softmmu/vl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4c1e94b00e..f0074845b7 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1552,9 +1552,7 @@ static Notifier qemu_unlink_pidfile_notifier;
 
 static void qemu_unlink_pidfile(Notifier *n, void *data)
 {
-    if (pid_file) {
-        unlink(pid_file);
-    }
+    unlink(pid_file);
 }
 
 static const QEMUOption *lookup_opt(int argc, char **argv,
@@ -2473,13 +2471,15 @@ static void qemu_maybe_daemonize(const char *pid_file)
     os_daemonize();
     rcu_disable_atfork();
 
-    if (pid_file && !qemu_write_pidfile(pid_file, &err)) {
-        error_reportf_err(err, "cannot create PID file: ");
-        exit(1);
-    }
+    if (pid_file) {
+        if (!qemu_write_pidfile(pid_file, &err)) {
+            error_reportf_err(err, "cannot create PID file: ");
+            exit(1);
+        }
 
-    qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
-    qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier);
+        qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
+        qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier);
+    }
 }
 
 static void qemu_init_displays(void)
-- 
2.35.3

Re: [PATCH 2/3] vl: Conditionally register PID file unlink notifier

[Daniel P. Berrangé]

On Thu, Jun 09, 2022 at 02:27:00PM +0200, Hanna Reitz wrote:
> Currently, the exit notifier for unlinking the PID file is registered
> unconditionally.  Limit it to only when we actually do create a PID
> file.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  softmmu/vl.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 3/3] vl: Unlink absolute PID file path

[Hanna Reitz]

After writing the PID file, we register an exit notifier to unlink it
when the process terminates.  However, if the process has changed its
working directory in the meantime (e.g. in os_setup_post() when
daemonizing), this will not work when the PID file path was relative.
Therefore, pass the absolute path (created with realpath()) to the
unlink() call in the exit notifier.

(realpath() needs a path pointing to an existing file, so we cannot use
it before qemu_write_pidfile().)

Reproducer:
$ cd /tmp
$ qemu-system-x86_64 --daemonize --pidfile qemu.pid
$ file qemu.pid
qemu.pid: ASCII text
$ kill $(cat qemu.pid)
$ file qemu.pid
qemu.pid: ASCII text

(qemu.pid should be gone after the process has terminated.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 softmmu/vl.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index f0074845b7..a97af525d1 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1548,11 +1548,18 @@ machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
 }
 
 static const char *pid_file;
-static Notifier qemu_unlink_pidfile_notifier;
+struct UnlinkPidfileNotifier {
+    Notifier notifier;
+    char *pid_file_realpath;
+};
+static struct UnlinkPidfileNotifier qemu_unlink_pidfile_notifier;
 
 static void qemu_unlink_pidfile(Notifier *n, void *data)
 {
-    unlink(pid_file);
+    struct UnlinkPidfileNotifier *upn;
+
+    upn = DO_UPCAST(struct UnlinkPidfileNotifier, notifier, n);
+    unlink(upn->pid_file_realpath);
 }
 
 static const QEMUOption *lookup_opt(int argc, char **argv,
@@ -2472,13 +2479,28 @@ static void qemu_maybe_daemonize(const char *pid_file)
     rcu_disable_atfork();
 
     if (pid_file) {
+        char *pid_file_realpath = NULL;
+
         if (!qemu_write_pidfile(pid_file, &err)) {
             error_reportf_err(err, "cannot create PID file: ");
             exit(1);
         }
 
-        qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
-        qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier);
+        pid_file_realpath = g_malloc0(PATH_MAX);
+        if (!realpath(pid_file, pid_file_realpath)) {
+            error_report("cannot resolve PID file path: %s: %s",
+                         pid_file, strerror(errno));
+            unlink(pid_file);
+            exit(1);
+        }
+
+        qemu_unlink_pidfile_notifier = (struct UnlinkPidfileNotifier) {
+            .notifier = {
+                .notify = qemu_unlink_pidfile,
+            },
+            .pid_file_realpath = pid_file_realpath,
+        };
+        qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier.notifier);
     }
 }
 
-- 
2.35.3

Re: [PATCH 3/3] vl: Unlink absolute PID file path

[Daniel P. Berrangé]

On Thu, Jun 09, 2022 at 02:27:01PM +0200, Hanna Reitz wrote:
> After writing the PID file, we register an exit notifier to unlink it
> when the process terminates.  However, if the process has changed its
> working directory in the meantime (e.g. in os_setup_post() when
> daemonizing), this will not work when the PID file path was relative.
> Therefore, pass the absolute path (created with realpath()) to the
> unlink() call in the exit notifier.
> 
> (realpath() needs a path pointing to an existing file, so we cannot use
> it before qemu_write_pidfile().)
> 
> Reproducer:
> $ cd /tmp
> $ qemu-system-x86_64 --daemonize --pidfile qemu.pid
> $ file qemu.pid
> qemu.pid: ASCII text
> $ kill $(cat qemu.pid)
> $ file qemu.pid
> qemu.pid: ASCII text
> 
> (qemu.pid should be gone after the process has terminated.)
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  softmmu/vl.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] qemu/qsd: Unlink absolute PID file path

[Hanna Reitz]

Ping

On 09.06.22 14:26, Hanna Reitz wrote:
> Hi,
>
> QEMU (the system emulator) and the storage daemon (QSD) write their PID
> to the given file when you specify --pidfile.  They keep the path around
> and register exit handlers (QEMU uses an exit notifier, QSD an atexit()
> function) to unlink this file when the process terminates.  These
> handlers unlink precisely the path that the user has specified via
> --pidfile, so if it was a relative path and the process has at any point
> changed its working directory, the path no longer points to the PID
> file, and so the unlink() will fail (or worse).
>
> When using --daemonize, the process will always change its working
> directory to /, so this problem basically always appears when using
> --daemonize and --pidfile in conjunction.
>
> (It gets even worse with QEMU’s --chroot, but I’m not sure whether
> there’s any trivial fix for that (whether chroot is used or not is
> confined to os-posix.c, so this would need to be externally visible; and
> I guess the plain would be to skip the unlink() in that case?), so I’d
> rather just skip that for now... :/)
>
> We can fix the problem by running realpath() once the PID file has been
> created, so we get an absolute path that we can unlink in the exit
> handler.  This is done here.
>
> (Another way to fix this might be to open the directory the PID file is
> in, keep the FD around, and use unlinkat() in the exit handler.  I
> couldn’t see any real benefit for this, though, so I didn’t go that
> route.  It might be beneficial for the --chroot case, but then again,
> precisely in that case we probably don’t want to keep random directory
> FDs around.)
>
>
> Hanna Reitz (3):
>    qsd: Unlink absolute PID file path
>    vl: Conditionally register PID file unlink notifier
>    vl: Unlink absolute PID file path
>
>   softmmu/vl.c                         | 42 +++++++++++++++++++++-------
>   storage-daemon/qemu-storage-daemon.c | 11 +++++++-
>   2 files changed, 42 insertions(+), 11 deletions(-)
>

Re: [PATCH 0/3] qemu/qsd: Unlink absolute PID file path

[Hanna Reitz]

On 09.06.22 14:26, Hanna Reitz wrote:
> Hi,
>
> QEMU (the system emulator) and the storage daemon (QSD) write their PID
> to the given file when you specify --pidfile.  They keep the path around
> and register exit handlers (QEMU uses an exit notifier, QSD an atexit()
> function) to unlink this file when the process terminates.  These
> handlers unlink precisely the path that the user has specified via
> --pidfile, so if it was a relative path and the process has at any point
> changed its working directory, the path no longer points to the PID
> file, and so the unlink() will fail (or worse).
>
> When using --daemonize, the process will always change its working
> directory to /, so this problem basically always appears when using
> --daemonize and --pidfile in conjunction.

[...]

> We can fix the problem by running realpath() once the PID file has been
> created, so we get an absolute path that we can unlink in the exit
> handler.  This is done here.

Thanks for the review, Dan, I’ve applied the series to my block branch:

https://gitlab.com/hreitz/qemu/-/commits/block