It is confusing having many different pieces of code enabling and
disabling commands, and it is not clear that they all have the same
semantics, especially wrt prioritization of the block/allow lists.

Centralizing the code in a single method "ga_apply_command_filters"
will provide a strong guarantee of consistency and clarify the
intended behaviour.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/main.c | 110 ++++++++++++++++++++++++++---------------------------
 1 file changed, 55 insertions(+), 55 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index e8f52f0794..c7b7b0a9bc 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -419,60 +419,79 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
     return strcmp(str1, str2);
 }
 
-/* disable commands that aren't safe for fsfreeze */
-static void ga_disable_not_allowed_freeze(const QmpCommand *cmd, void *opaque)
+static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
 {
-    bool allowed = false;
     int i = 0;
+    GAConfig *config = state->config;
     const char *name = qmp_command_name(cmd);
+    /* Fallback policy is allow everything */
+    bool allowed = true;
 
-    while (ga_freeze_allowlist[i] != NULL) {
-        if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
+    if (config->allowedrpcs) {
+        /*
+         * If an allow-list is given, this changes the fallback
+         * policy to deny everything
+         */
+        allowed = false;
+
+        if (g_list_find_custom(config->allowedrpcs, name, ga_strcmp) != NULL) {
             allowed = true;
         }
-        i++;
-    }
-    if (!allowed) {
-        g_debug("disabling command: %s", name);
-        qmp_disable_command(&ga_commands, name, "the agent is in frozen state");
     }
-}
-
-/* [re-]enable all commands, except those explicitly blocked by user */
-static void ga_enable_non_blocked(const QmpCommand *cmd, void *opaque)
-{
-    GAState *s = opaque;
-    GList *blockedrpcs = s->blockedrpcs;
-    GList *allowedrpcs = s->allowedrpcs;
-    const char *name = qmp_command_name(cmd);
 
-    if (g_list_find_custom(blockedrpcs, name, ga_strcmp) == NULL) {
-        if (qmp_command_is_enabled(cmd)) {
-            return;
+    /*
+     * If both allowedrpcs and blockedrpcs are set, the blocked
+     * list will take priority
+     */
+    if (config->blockedrpcs) {
+        if (g_list_find_custom(config->blockedrpcs, name, ga_strcmp) != NULL) {
+            allowed = false;
         }
+    }
 
-        if (allowedrpcs &&
-            g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
-            return;
-        }
+    /*
+     * If frozen, this filtering must take priority over
+     * absolutely everything
+     */
+    if (state->frozen) {
+        allowed = false;
 
-        g_debug("enabling command: %s", name);
-        qmp_enable_command(&ga_commands, name);
+        while (ga_freeze_allowlist[i] != NULL) {
+            if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
+                allowed = true;
+            }
+            i++;
+        }
     }
+
+    return allowed;
 }
 
-/* disable commands that aren't allowed */
-static void ga_disable_not_allowed(const QmpCommand *cmd, void *opaque)
+static void ga_apply_command_filters_iter(const QmpCommand *cmd, void *opaque)
 {
-    GList *allowedrpcs = opaque;
+    GAState *state = opaque;
+    bool want = ga_command_is_allowed(cmd, state);
+    bool have = qmp_command_is_enabled(cmd);
     const char *name = qmp_command_name(cmd);
 
-    if (g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
+    if (want == have) {
+        return;
+    }
+
+    if (qmp_command_is_enabled(cmd)) {
         g_debug("disabling command: %s", name);
         qmp_disable_command(&ga_commands, name, "the command is not allowed");
+    } else {
+        g_debug("enabling command: %s", name);
+        qmp_enable_command(&ga_commands, name);
     }
 }
 
+static void ga_apply_command_filters(GAState *state)
+{
+    qmp_for_each_command(&ga_commands, ga_apply_command_filters_iter, state);
+}
+
 static bool ga_create_file(const char *path)
 {
     int fd = open(path, O_CREAT | O_WRONLY, S_IWUSR | S_IRUSR);
@@ -505,15 +524,14 @@ void ga_set_frozen(GAState *s)
     if (ga_is_frozen(s)) {
         return;
     }
-    /* disable all forbidden (for frozen state) commands */
-    qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze, NULL);
     g_warning("disabling logging due to filesystem freeze");
-    ga_disable_logging(s);
     s->frozen = true;
     if (!ga_create_file(s->state_filepath_isfrozen)) {
         g_warning("unable to create %s, fsfreeze may not function properly",
                   s->state_filepath_isfrozen);
     }
+    ga_apply_command_filters(s);
+    ga_disable_logging(s);
 }
 
 void ga_unset_frozen(GAState *s)
@@ -545,12 +563,12 @@ void ga_unset_frozen(GAState *s)
     }
 
     /* enable all disabled, non-blocked and allowed commands */
-    qmp_for_each_command(&ga_commands, ga_enable_non_blocked, s);
     s->frozen = false;
     if (!ga_delete_file(s->state_filepath_isfrozen)) {
         g_warning("unable to delete %s, fsfreeze may not function properly",
                   s->state_filepath_isfrozen);
     }
+    ga_apply_command_filters(s);
 }
 
 #ifdef CONFIG_FSFREEZE
@@ -1414,7 +1432,6 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
             s->deferred_options.log_filepath = config->log_filepath;
         }
         ga_disable_logging(s);
-        qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze, NULL);
     } else {
         if (config->daemonize) {
             become_daemon(config->pid_filepath);
@@ -1438,25 +1455,8 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
         return NULL;
     }
 
-    if (config->allowedrpcs) {
-        qmp_for_each_command(&ga_commands, ga_disable_not_allowed, config->allowedrpcs);
-        s->allowedrpcs = config->allowedrpcs;
-    }
+    ga_apply_command_filters(s);
 
-    /*
-     * Some commands can be blocked due to system limitation.
-     * Initialize blockedrpcs list even if allowedrpcs specified.
-     */
-    config->blockedrpcs = ga_command_init_blockedrpcs(config->blockedrpcs);
-    if (config->blockedrpcs) {
-        GList *l = config->blockedrpcs;
-        s->blockedrpcs = config->blockedrpcs;
-        do {
-            g_debug("disabling command: %s", (char *)l->data);
-            qmp_disable_command(&ga_commands, l->data, NULL);
-            l = g_list_next(l);
-        } while (l);
-    }
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-- 
2.45.1

Hi

On Tue, Jun 4, 2024 at 5:51 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> It is confusing having many different pieces of code enabling and
> disabling commands, and it is not clear that they all have the same
> semantics, especially wrt prioritization of the block/allow lists.
>
> Centralizing the code in a single method "ga_apply_command_filters"
> will provide a strong guarantee of consistency and clarify the
> intended behaviour.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

The clean up is very much welcome and looks correct, but it crashes:

Thread 1 "qemu-ga" received signal SIGSEGV, Segmentation fault.
0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800,
state=0x555555633710) at ../qga/main.c:430
430    if (config->allowedrpcs) {
(gdb) bt
#0  0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800,
state=0x555555633710) at ../qga/main.c:430
#1  ga_apply_command_filters_iter (cmd=0x555555632800,
opaque=0x555555633710) at ../qga/main.c:473
#2  0x000055555559ef81 in qmp_for_each_command (cmds=cmds@entry=0x55555562c2b0
<ga_commands>, fn=fn@entry=0x55555557db30 <ga_apply_command_filters_iter>,
opaque=opaque@entry=0x555555633710)
    at ../qapi/qmp-registry.c:93
#3  0x0000555555571436 in ga_apply_command_filters (state=0x555555633710)
at ../qga/main.c:492
#4  initialize_agent (config=0x555555632760, socket_activation=0) at
../qga/main.c:1452
#5  main (argc=<optimized out>, argv=<optimized out>) at ../qga/main.c:1646
(gdb) p state.config
$1 = (GAConfig *) 0x0

(meson test fails too)

I wonder why s->config is set so late in initialize_agent(). Moving it
earlier seems to solve the issue, but reviewing all code paths is tedious..


---
>  qga/main.c | 110 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 55 insertions(+), 55 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index e8f52f0794..c7b7b0a9bc 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -419,60 +419,79 @@ static gint ga_strcmp(gconstpointer str1,
> gconstpointer str2)
>      return strcmp(str1, str2);
>  }
>
> -/* disable commands that aren't safe for fsfreeze */
> -static void ga_disable_not_allowed_freeze(const QmpCommand *cmd, void
> *opaque)
> +static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
>  {
> -    bool allowed = false;
>      int i = 0;
> +    GAConfig *config = state->config;
>      const char *name = qmp_command_name(cmd);
> +    /* Fallback policy is allow everything */
> +    bool allowed = true;
>
> -    while (ga_freeze_allowlist[i] != NULL) {
> -        if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
> +    if (config->allowedrpcs) {
> +        /*
> +         * If an allow-list is given, this changes the fallback
> +         * policy to deny everything
> +         */
> +        allowed = false;
> +
> +        if (g_list_find_custom(config->allowedrpcs, name, ga_strcmp) !=
> NULL) {
>              allowed = true;
>          }
> -        i++;
> -    }
> -    if (!allowed) {
> -        g_debug("disabling command: %s", name);
> -        qmp_disable_command(&ga_commands, name, "the agent is in frozen
> state");
>      }
> -}
> -
> -/* [re-]enable all commands, except those explicitly blocked by user */
> -static void ga_enable_non_blocked(const QmpCommand *cmd, void *opaque)
> -{
> -    GAState *s = opaque;
> -    GList *blockedrpcs = s->blockedrpcs;
> -    GList *allowedrpcs = s->allowedrpcs;
> -    const char *name = qmp_command_name(cmd);
>
> -    if (g_list_find_custom(blockedrpcs, name, ga_strcmp) == NULL) {
> -        if (qmp_command_is_enabled(cmd)) {
> -            return;
> +    /*
> +     * If both allowedrpcs and blockedrpcs are set, the blocked
> +     * list will take priority
> +     */
> +    if (config->blockedrpcs) {
> +        if (g_list_find_custom(config->blockedrpcs, name, ga_strcmp) !=
> NULL) {
> +            allowed = false;
>          }
> +    }
>
> -        if (allowedrpcs &&
> -            g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
> -            return;
> -        }
> +    /*
> +     * If frozen, this filtering must take priority over
> +     * absolutely everything
> +     */
> +    if (state->frozen) {
> +        allowed = false;
>
> -        g_debug("enabling command: %s", name);
> -        qmp_enable_command(&ga_commands, name);
> +        while (ga_freeze_allowlist[i] != NULL) {
> +            if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
> +                allowed = true;
> +            }
> +            i++;
> +        }
>      }
> +
> +    return allowed;
>  }
>
> -/* disable commands that aren't allowed */
> -static void ga_disable_not_allowed(const QmpCommand *cmd, void *opaque)
> +static void ga_apply_command_filters_iter(const QmpCommand *cmd, void
> *opaque)
>  {
> -    GList *allowedrpcs = opaque;
> +    GAState *state = opaque;
> +    bool want = ga_command_is_allowed(cmd, state);
> +    bool have = qmp_command_is_enabled(cmd);
>      const char *name = qmp_command_name(cmd);
>
> -    if (g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
> +    if (want == have) {
> +        return;
> +    }
> +
> +    if (qmp_command_is_enabled(cmd)) {
>          g_debug("disabling command: %s", name);
>          qmp_disable_command(&ga_commands, name, "the command is not
> allowed");
> +    } else {
> +        g_debug("enabling command: %s", name);
> +        qmp_enable_command(&ga_commands, name);
>      }
>  }
>
> +static void ga_apply_command_filters(GAState *state)
> +{
> +    qmp_for_each_command(&ga_commands, ga_apply_command_filters_iter,
> state);
> +}
> +
>  static bool ga_create_file(const char *path)
>  {
>      int fd = open(path, O_CREAT | O_WRONLY, S_IWUSR | S_IRUSR);
> @@ -505,15 +524,14 @@ void ga_set_frozen(GAState *s)
>      if (ga_is_frozen(s)) {
>          return;
>      }
> -    /* disable all forbidden (for frozen state) commands */
> -    qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze,
> NULL);
>      g_warning("disabling logging due to filesystem freeze");
> -    ga_disable_logging(s);
>      s->frozen = true;
>      if (!ga_create_file(s->state_filepath_isfrozen)) {
>          g_warning("unable to create %s, fsfreeze may not function
> properly",
>                    s->state_filepath_isfrozen);
>      }
> +    ga_apply_command_filters(s);
> +    ga_disable_logging(s);
>  }
>
>  void ga_unset_frozen(GAState *s)
> @@ -545,12 +563,12 @@ void ga_unset_frozen(GAState *s)
>      }
>
>      /* enable all disabled, non-blocked and allowed commands */
> -    qmp_for_each_command(&ga_commands, ga_enable_non_blocked, s);
>      s->frozen = false;
>      if (!ga_delete_file(s->state_filepath_isfrozen)) {
>          g_warning("unable to delete %s, fsfreeze may not function
> properly",
>                    s->state_filepath_isfrozen);
>      }
> +    ga_apply_command_filters(s);
>  }
>
>  #ifdef CONFIG_FSFREEZE
> @@ -1414,7 +1432,6 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
>              s->deferred_options.log_filepath = config->log_filepath;
>          }
>          ga_disable_logging(s);
> -        qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze,
> NULL);
>      } else {
>          if (config->daemonize) {
>              become_daemon(config->pid_filepath);
> @@ -1438,25 +1455,8 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
>          return NULL;
>      }
>
> -    if (config->allowedrpcs) {
> -        qmp_for_each_command(&ga_commands, ga_disable_not_allowed,
> config->allowedrpcs);
> -        s->allowedrpcs = config->allowedrpcs;
> -    }
> +    ga_apply_command_filters(s);
>
> -    /*
> -     * Some commands can be blocked due to system limitation.
> -     * Initialize blockedrpcs list even if allowedrpcs specified.
> -     */
> -    config->blockedrpcs =
> ga_command_init_blockedrpcs(config->blockedrpcs);
> -    if (config->blockedrpcs) {
> -        GList *l = config->blockedrpcs;
> -        s->blockedrpcs = config->blockedrpcs;
> -        do {
> -            g_debug("disabling command: %s", (char *)l->data);
> -            qmp_disable_command(&ga_commands, l->data, NULL);
> -            l = g_list_next(l);
> -        } while (l);
> -    }
>      s->command_state = ga_command_state_new();
>      ga_command_state_init(s, s->command_state);
>      ga_command_state_init_all(s->command_state);
> --
> 2.45.1
>
>
>

-- 
Marc-André Lureau

On Wed, Jun 05, 2024 at 02:37:24PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jun 4, 2024 at 5:51 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > It is confusing having many different pieces of code enabling and
> > disabling commands, and it is not clear that they all have the same
> > semantics, especially wrt prioritization of the block/allow lists.
> >
> > Centralizing the code in a single method "ga_apply_command_filters"
> > will provide a strong guarantee of consistency and clarify the
> > intended behaviour.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> 
> The clean up is very much welcome and looks correct, but it crashes:
> 
> Thread 1 "qemu-ga" received signal SIGSEGV, Segmentation fault.
> 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800,
> state=0x555555633710) at ../qga/main.c:430
> 430    if (config->allowedrpcs) {
> (gdb) bt
> #0  0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800,
> state=0x555555633710) at ../qga/main.c:430
> #1  ga_apply_command_filters_iter (cmd=0x555555632800,
> opaque=0x555555633710) at ../qga/main.c:473
> #2  0x000055555559ef81 in qmp_for_each_command (cmds=cmds@entry=0x55555562c2b0
> <ga_commands>, fn=fn@entry=0x55555557db30 <ga_apply_command_filters_iter>,
> opaque=opaque@entry=0x555555633710)
>     at ../qapi/qmp-registry.c:93
> #3  0x0000555555571436 in ga_apply_command_filters (state=0x555555633710)
> at ../qga/main.c:492
> #4  initialize_agent (config=0x555555632760, socket_activation=0) at
> ../qga/main.c:1452
> #5  main (argc=<optimized out>, argv=<optimized out>) at ../qga/main.c:1646
> (gdb) p state.config
> $1 = (GAConfig *) 0x0
> 
> (meson test fails too)
> 
> I wonder why s->config is set so late in initialize_agent(). Moving it
> earlier seems to solve the issue, but reviewing all code paths is tedious..

The ga_apply_command_filters() call can just be moved later,
since the only constraint is that is called /before/ we call
g_main_loop_run() to start processing I/O


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 :|

Hi

On Wed, Jun 5, 2024 at 2:37 PM Marc-André Lureau <marcandre.lureau@gmail.com>
wrote:

> Hi
>
> On Tue, Jun 4, 2024 at 5:51 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> It is confusing having many different pieces of code enabling and
>> disabling commands, and it is not clear that they all have the same
>> semantics, especially wrt prioritization of the block/allow lists.
>>
>> Centralizing the code in a single method "ga_apply_command_filters"
>> will provide a strong guarantee of consistency and clarify the
>> intended behaviour.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>
>
> The clean up is very much welcome and looks correct, but it crashes:
>
> Thread 1 "qemu-ga" received signal SIGSEGV, Segmentation fault.
> 0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800,
> state=0x555555633710) at ../qga/main.c:430
> 430    if (config->allowedrpcs) {
> (gdb) bt
> #0  0x000055555557db4f in ga_command_is_allowed (cmd=0x555555632800,
> state=0x555555633710) at ../qga/main.c:430
> #1  ga_apply_command_filters_iter (cmd=0x555555632800,
> opaque=0x555555633710) at ../qga/main.c:473
> #2  0x000055555559ef81 in qmp_for_each_command (cmds=cmds@entry=0x55555562c2b0
> <ga_commands>, fn=fn@entry=0x55555557db30
> <ga_apply_command_filters_iter>, opaque=opaque@entry=0x555555633710)
>     at ../qapi/qmp-registry.c:93
> #3  0x0000555555571436 in ga_apply_command_filters (state=0x555555633710)
> at ../qga/main.c:492
> #4  initialize_agent (config=0x555555632760, socket_activation=0) at
> ../qga/main.c:1452
> #5  main (argc=<optimized out>, argv=<optimized out>) at ../qga/main.c:1646
> (gdb) p state.config
> $1 = (GAConfig *) 0x0
>
> (meson test fails too)
>
> I wonder why s->config is set so late in initialize_agent(). Moving it
> earlier seems to solve the issue, but reviewing all code paths is tedious..
>

Actually, there seems to be few ->config users, and they don't check if
it's NULL. So I guess it's ok to move it earlier.

-- 
Marc-André Lureau