| Message ID | 20230227183505.280368-1-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] test-vmstate: fix bad GTree usage, use-after-free | expand |
On Mon, Feb 27, 2023 at 07:35:05PM +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)." It might be worth noting that this bug only happens now because glib2 remove their custom slice allocator and switched to using system malloc. With glibc + MALLOC_PERTURB_, malloc will find these kinds of bugs. The relevant glib2 change that causes the problem is: https://gitlab.gnome.org/GNOME/glib/-/commit/45b5a6c1e56d5b73cc5ed798ef59a5601e56c170 > A SIGSEV can be observed while running test-vmstate. SIGSEGV > 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> You can add: Tested-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> > --- > > This is a 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 SIGSEV 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
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)
According to g_tree_foreach() documentation: "The tree may not be modified while iterating over it (you can't add/remove items)." A SIGSEV can be observed while running test-vmstate. 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> --- This is a 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 SIGSEV --- tests/unit/test-vmstate.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)