[PATCH] qga: fix crash when a signal arrives during shutdown

[PATCH] qga: fix crash when a signal arrives during shutdown

[Brad Smith]

qga: fix crash when a signal arrives during shutdown

quit_handler() stays installed for SIGINT and SIGTERM for the life
of the process and dereferences the global ga_state. cleanup_agent()
frees the agent state and clears ga_state, so a SIGINT or SIGTERM
arriving after cleanup makes quit_handler() dereference NULL in
ga_is_frozen() and the agent crashes with SIGSEGV.

This happens in practice when qemu-ga runs under something that
forwards signals. For example, running it under sudo and stopping it
with "pkill -f" signals both sudo and qemu-ga, so the agent receives
SIGTERM twice.

Reproduced on FreeBSD by sending SIGTERM after cleanup_agent() under
lldb.

Return early from quit_handler() when ga_state is NULL, and clear
ga_state as soon as the command state cleanup is done, before the
main loop and the state are freed, so a signal during the rest of
cleanup does not touch freed memory. ga_state cannot be cleared
earlier, as command cleanup callbacks such as
guest_fsfreeze_cleanup() still use it.

Signed-off-by: Brad Smith <brad@comstyle.com>
---
 qga/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index fd19c7037d..8cc9810572 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -167,6 +167,10 @@ init_dfl_pathnames(void)
 
 static void quit_handler(int sig)
 {
+    if (!ga_state) {
+        return;
+    }
+
     /* if we're frozen, don't exit unless we're absolutely forced to,
      * because it's basically impossible for graceful exit to complete
      * unless all log/pid files are on unfreezable filesystems. there's
@@ -1589,13 +1593,13 @@ static void cleanup_agent(GAState *s)
         ga_command_state_free(s->command_state);
         json_message_parser_destroy(&s->parser);
     }
+    ga_state = NULL;
     g_free(s->pstate_filepath);
     g_free(s->state_filepath_isfrozen);
     if (s->main_loop) {
         g_main_loop_unref(s->main_loop);
     }
     g_free(s);
-    ga_state = NULL;
 }
 
 static int run_agent_once(GAState *s)
-- 
2.56.0

Re: [PATCH] qga: fix crash when a signal arrives during shutdown

[Roman Bogorodskiy]

  Brad Smith wrote:

> qga: fix crash when a signal arrives during shutdown

For some reason your commit message bodies duplicate the title.

> quit_handler() stays installed for SIGINT and SIGTERM for the life
> of the process and dereferences the global ga_state. cleanup_agent()
> frees the agent state and clears ga_state, so a SIGINT or SIGTERM
> arriving after cleanup makes quit_handler() dereference NULL in
> ga_is_frozen() and the agent crashes with SIGSEGV.
> 
> This happens in practice when qemu-ga runs under something that
> forwards signals. For example, running it under sudo and stopping it
> with "pkill -f" signals both sudo and qemu-ga, so the agent receives
> SIGTERM twice.
> 
> Reproduced on FreeBSD by sending SIGTERM after cleanup_agent() under
> lldb.
> 
> Return early from quit_handler() when ga_state is NULL, and clear
> ga_state as soon as the command state cleanup is done, before the
> main loop and the state are freed, so a signal during the rest of
> cleanup does not touch freed memory. ga_state cannot be cleared
> earlier, as command cleanup callbacks such as
> guest_fsfreeze_cleanup() still use it.
> 
> Signed-off-by: Brad Smith <brad@comstyle.com>
> ---
>  qga/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index fd19c7037d..8cc9810572 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -167,6 +167,10 @@ init_dfl_pathnames(void)
>  
>  static void quit_handler(int sig)
>  {
> +    if (!ga_state) {
> +        return;
> +    }
> +
>      /* if we're frozen, don't exit unless we're absolutely forced to,
>       * because it's basically impossible for graceful exit to complete
>       * unless all log/pid files are on unfreezable filesystems. there's
> @@ -1589,13 +1593,13 @@ static void cleanup_agent(GAState *s)
>          ga_command_state_free(s->command_state);
>          json_message_parser_destroy(&s->parser);
>      }
> +    ga_state = NULL;
>      g_free(s->pstate_filepath);
>      g_free(s->state_filepath_isfrozen);
>      if (s->main_loop) {
>          g_main_loop_unref(s->main_loop);
>      }
>      g_free(s);
> -    ga_state = NULL;
>  }
>  
>  static int run_agent_once(GAState *s)
> -- 
> 2.56.0
> 

I was testing this change by sending SIGTERM in a loop and noticed that
it still crashes with SIGSEGV or SIGARBT from time to time. I was able
to fix that by ignoring signals in the clean up routines:

diff --git a/qga/main.c b/qga/main.c
index 8cc9810572..47686bca80 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -167,6 +167,12 @@ init_dfl_pathnames(void)
 
 static void quit_handler(int sig)
 {
+#ifndef _WIN32
+    struct sigaction action = {
+        .sa_handler = SIG_IGN,
+    };
+#endif
+
     if (!ga_state) {
         return;
     }
@@ -203,6 +209,13 @@ static void quit_handler(int sig)
         return;
 #endif
     }
+
+#ifndef _WIN32
+    sigemptyset(&action.sa_mask);
+    sigaction(SIGINT, &action, NULL);
+    sigaction(SIGTERM, &action, NULL);
+#endif
+
     g_debug("received signal num %d, quitting", sig);
 
     stop_agent(ga_state, true);
@@ -1587,6 +1600,18 @@ static void cleanup_agent(GAState *s)
     if (s->load_avg_pdh_query) {
         PdhCloseQuery(s->load_avg_pdh_query);
     }
+#else
+    struct sigaction action = {
+        .sa_handler = SIG_IGN,
+    };
+
+    sigemptyset(&action.sa_mask);
+    if (sigaction(SIGINT, &action, NULL) != 0) {
+        g_warning("error configuring signal handler: %s", strerror(errno));
+    }
+    if (sigaction(SIGTERM, &action, NULL) != 0) {
+        g_warning("error configuring signal handler: %s", strerror(errno));
+    }
 #endif
     if (s->command_state) {
         ga_command_state_cleanup_all(s->command_state);