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.lur...@redhat.com> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> Reported-by: Richard W.M. Jones <rjo...@redhat.com> >> Tested-by: Richard W.M. Jones <rjo...@redhat.com> >> Reviewed-by: Richard W.M. Jones <rjo...@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.lur...@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é <berra...@redhat.com> > > With regards, > Daniel
