Re: [PATCH v3] test-vmstate: fix bad GTree usage, use-after-free

[Daniel P . Berrangé]

On Tue, Feb 28, 2023 at 10:03:52AM +0100, Eric Auger wrote:
> According to g_tree_foreach() documentation:
> "The tree may not be modified while iterating over it (you can't
> add/remove items)."
> 
> Since glib2 has removed its custom slice allocator and has switched
> to using system malloc, a SIGSEGV can be observed while running
> test-vmstate. With glibc + MALLOC_PERTURB_, malloc is able to detect
> this kind of bugs. The relevant glib2 change that causes the problem
> is:

IMHO this somewhat reads like we're blaming glib2 for a causing
a bug in our own code. Can we change that paragraph to something
more like

  By missing the requirement to not modify the tree, the QEMU
  test case has been using memory after it was freed. Historically
  GLib2 used a slice allocator for the GTree APIs which did not
  immediately release the memory back to the system allocator.
  As a result QEMU's use-after-free bug was not visible. With
  GLib > 2.75.3, the slice allocator has been removed, such that
  all allocations/frees are directly handled by the system
  allocator, exposing the problematic iteration code.

> https://gitlab.gnome.org/GNOME/glib/-/commit/45b5a6c1e56d5b73cc5ed798ef59a5601e56c170
> 
> Get rid of the node removal within the tree traversal. Also
> check the trees have the same number of nodes before the actual
> diff.
> 
> Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1518
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 
> ---
> 
> v2 -> v3:
> - Enhance the commit message with Rich's explanations
> 
> v1 -> v2:
> - respin of Marc-André's patch from Aug 2020, which can be
> found at
> https://lore.kernel.org/qemu-devel/20200827161826.1165971-1-marcandre.lureau@redhat.com/
> This fell through the cracks and now we hit a SIGSEGV
> ---
>  tests/unit/test-vmstate.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> index 79357b29ca..0b7d5ecd68 100644
> --- a/tests/unit/test-vmstate.c
> +++ b/tests/unit/test-vmstate.c
> @@ -1073,7 +1073,6 @@ static gboolean diff_tree(gpointer key, gpointer value, 
> gpointer data)
>      struct match_node_data d = {tp->tree2, key, value};
>  
>      g_tree_foreach(tp->tree2, tp->match_node, &d);
> -    g_tree_remove(tp->tree1, key);
>      return false;
>  }
>  
> @@ -1082,9 +1081,9 @@ static void compare_trees(GTree *tree1, GTree *tree2,
>  {
>      struct tree_cmp_data tp = {tree1, tree2, function};
>  
> +    assert(g_tree_nnodes(tree1) == g_tree_nnodes(tree2));
>      g_tree_foreach(tree1, diff_tree, &tp);
> -    assert(g_tree_nnodes(tree1) == 0);
> -    assert(g_tree_nnodes(tree2) == 0);
> +    g_tree_destroy(g_tree_ref(tree1));
>  }
>  
>  static void diff_domain(TestGTreeDomain *d1, TestGTreeDomain *d2)

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