| Message ID | 20230228090352.339466-1-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] test-vmstate: fix bad GTree usage, use-after-free | expand |
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
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
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)." 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(-)