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

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

[Eric Auger]

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:

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)
-- 
2.38.1

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

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

[Eric Auger]

Hi Daniel,

On 2/28/23 10:12, Daniel P. Berrangé wrote:
> 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.

agreed. Changed the wording in the commit message.

Thanks

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