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

[Eric Auger <eric.auger@redhat.com>]

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