qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] test-vmstate: fix bad GTree usage, use-after-free
@ 2020-08-27 16:18 marcandre.lureau
  2020-08-27 16:28 ` Juan Quintela
  2020-08-27 16:34 ` Auger Eric
  0 siblings, 2 replies; 6+ messages in thread
From: marcandre.lureau @ 2020-08-27 16:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, Eric Auger, quintela

From: Marc-André Lureau <marcandre.lureau@redhat.com>

According to g_tree_foreach() documentation:
"The tree may not be modified while iterating over it (you can't
add/remove items)."

Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
Cc: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-vmstate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index f7b3868881..31aefa78f0 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -1078,7 +1078,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;
 }
 
@@ -1088,7 +1087,7 @@ static void compare_trees(GTree *tree1, GTree *tree2,
     struct tree_cmp_data tp = {tree1, tree2, function};
 
     g_tree_foreach(tree1, diff_tree, &tp);
-    assert(g_tree_nnodes(tree1) == 0);
+    g_tree_destroy(g_tree_ref(tree1));
     assert(g_tree_nnodes(tree2) == 0);
 }
 
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] test-vmstate: fix bad GTree usage, use-after-free
  2020-08-27 16:18 [PATCH] test-vmstate: fix bad GTree usage, use-after-free marcandre.lureau
@ 2020-08-27 16:28 ` Juan Quintela
  2020-08-27 16:34 ` Auger Eric
  1 sibling, 0 replies; 6+ messages in thread
From: Juan Quintela @ 2020-08-27 16:28 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: Eric Auger, qemu-devel

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> According to g_tree_foreach() documentation:
> "The tree may not be modified while iterating over it (you can't
> add/remove items)."
>
> Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
> Cc: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test-vmstate: fix bad GTree usage, use-after-free
  2020-08-27 16:18 [PATCH] test-vmstate: fix bad GTree usage, use-after-free marcandre.lureau
  2020-08-27 16:28 ` Juan Quintela
@ 2020-08-27 16:34 ` Auger Eric
  2020-08-27 18:35   ` Marc-André Lureau
  1 sibling, 1 reply; 6+ messages in thread
From: Auger Eric @ 2020-08-27 16:34 UTC (permalink / raw)
  To: marcandre.lureau, qemu-devel; +Cc: quintela

Hi Marc-Andre

On 8/27/20 6:18 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> According to g_tree_foreach() documentation:
> "The tree may not be modified while iterating over it (you can't
> add/remove items)."

Hum I did not see that.
> 
> Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
> Cc: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-vmstate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index f7b3868881..31aefa78f0 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -1078,7 +1078,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);
it does not test the same thing I am afraid. If one of the trees
contains more elements than the others this won't be detected.

Also there is another case of removal inside the traversal in the
match_node(): in match_interval_mapping_node() and match_domain_node()

Thanks

Eric


>      return false;
>  }
>  
> @@ -1088,7 +1087,7 @@ static void compare_trees(GTree *tree1, GTree *tree2,
>      struct tree_cmp_data tp = {tree1, tree2, function};
>  
>      g_tree_foreach(tree1, diff_tree, &tp);
> -    assert(g_tree_nnodes(tree1) == 0);> +    g_tree_destroy(g_tree_ref(tree1));
>      assert(g_tree_nnodes(tree2) == 0);
>  }
>  
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test-vmstate: fix bad GTree usage, use-after-free
  2020-08-27 16:34 ` Auger Eric
@ 2020-08-27 18:35   ` Marc-André Lureau
  2020-08-28  7:19     ` Auger Eric
  0 siblings, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2020-08-27 18:35 UTC (permalink / raw)
  To: Auger Eric; +Cc: QEMU, Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Hi

On Thu, Aug 27, 2020 at 8:34 PM Auger Eric <eric.auger@redhat.com> wrote:

> Hi Marc-Andre
>
> On 8/27/20 6:18 PM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > According to g_tree_foreach() documentation:
> > "The tree may not be modified while iterating over it (you can't
> > add/remove items)."
>
> Hum I did not see that.
> >
> > Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/test-vmstate.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> > index f7b3868881..31aefa78f0 100644
> > --- a/tests/test-vmstate.c
> > +++ b/tests/test-vmstate.c
> > @@ -1078,7 +1078,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);
> it does not test the same thing I am afraid. If one of the trees
> contains more elements than the others this won't be detected.
>
>
-    assert(g_tree_nnodes(tree1) == 0);

Was simply checking that all nodes from tree1 were deleted.

Whatever else must have been checked elsewhere or differently by new code.


> Also there is another case of removal inside the traversal in the
> match_node(): in match_interval_mapping_node() and match_domain_node()
>
>
Yes, but they don't update the traversed tree.

Thanks
>
> Eric
>
>
> >      return false;
> >  }
> >
> > @@ -1088,7 +1087,7 @@ static void compare_trees(GTree *tree1, GTree
> *tree2,
> >      struct tree_cmp_data tp = {tree1, tree2, function};
> >
> >      g_tree_foreach(tree1, diff_tree, &tp);
> > -    assert(g_tree_nnodes(tree1) == 0);> +
> g_tree_destroy(g_tree_ref(tree1));
> >      assert(g_tree_nnodes(tree2) == 0);
> >  }
> >
> >
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3427 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test-vmstate: fix bad GTree usage, use-after-free
  2020-08-27 18:35   ` Marc-André Lureau
@ 2020-08-28  7:19     ` Auger Eric
  2020-08-28  7:52       ` Auger Eric
  0 siblings, 1 reply; 6+ messages in thread
From: Auger Eric @ 2020-08-28  7:19 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU, Juan Quintela

Hi Marc-André,

On 8/27/20 8:35 PM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 27, 2020 at 8:34 PM Auger Eric <eric.auger@redhat.com
> <mailto:eric.auger@redhat.com>> wrote:
> 
>     Hi Marc-Andre
> 
>     On 8/27/20 6:18 PM, marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com> wrote:
>     > From: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
>     >
>     > According to g_tree_foreach() documentation:
>     > "The tree may not be modified while iterating over it (you can't
>     > add/remove items)."
> 
>     Hum I did not see that.
>     >
>     > Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
>     > Cc: Eric Auger <eric.auger@redhat.com <mailto:eric.auger@redhat.com>>
>     > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
>     > ---
>     >  tests/test-vmstate.c | 3 +--
>     >  1 file changed, 1 insertion(+), 2 deletions(-)
>     >
>     > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
>     > index f7b3868881..31aefa78f0 100644
>     > --- a/tests/test-vmstate.c
>     > +++ b/tests/test-vmstate.c
>     > @@ -1078,7 +1078,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);
>     it does not test the same thing I am afraid. If one of the trees
>     contains more elements than the others this won't be detected.
> 
> 
> -    assert(g_tree_nnodes(tree1) == 0);
> 
> Was simply checking that all nodes from tree1 were deleted.
> 
> Whatever else must have been checked elsewhere or differently by new code.
compare_trees() iterates on tree1 and tries to find a fellow node in
tree2 for each node in tree1. At least we need to check that the number
of nodes is the same in both trees otherwise tree2 can have more nodes
than tree1 and with the change this won't be detected.
>  
> 
>     Also there is another case of removal inside the traversal in the
>     match_node(): in match_interval_mapping_node() and match_domain_node()
> 
> 
> Yes, but they don't update the traversed tree.
Hum yes, you mean we exit the loop by returning true in the match
function so that's OK.

So your modif + replacing
    g_tree_foreach(tree1, diff_tree, &tp);
    assert(g_tree_nnodes(tree1) == 0);
    assert(g_tree_nnodes(tree2) == 0);

by

assert(g_tree_nnodes(tree1) == g_tree_nnodes(tree2));
g_tree_foreach(tree1, diff_tree, &tp);

should do the job, no?

Thanks

Eric


> 
>     Thanks
> 
>     Eric
> 
> 
>     >      return false;
>     >  }
>     > 
>     > @@ -1088,7 +1087,7 @@ static void compare_trees(GTree *tree1,
>     GTree *tree2,
>     >      struct tree_cmp_data tp = {tree1, tree2, function};
>     > 
>     >      g_tree_foreach(tree1, diff_tree, &tp);
>     > -    assert(g_tree_nnodes(tree1) == 0);> +   
>     g_tree_destroy(g_tree_ref(tree1));
>     >      assert(g_tree_nnodes(tree2) == 0);
>     >  }
>     > 
>     >
> 
> 
> 
> 
> -- 
> Marc-André Lureau



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test-vmstate: fix bad GTree usage, use-after-free
  2020-08-28  7:19     ` Auger Eric
@ 2020-08-28  7:52       ` Auger Eric
  0 siblings, 0 replies; 6+ messages in thread
From: Auger Eric @ 2020-08-28  7:52 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU, Juan Quintela

Hi Marc-André,

On 8/28/20 9:19 AM, Auger Eric wrote:
> Hi Marc-André,
> 
> On 8/27/20 8:35 PM, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Aug 27, 2020 at 8:34 PM Auger Eric <eric.auger@redhat.com
>> <mailto:eric.auger@redhat.com>> wrote:
>>
>>     Hi Marc-Andre
>>
>>     On 8/27/20 6:18 PM, marcandre.lureau@redhat.com
>>     <mailto:marcandre.lureau@redhat.com> wrote:
>>     > From: Marc-André Lureau <marcandre.lureau@redhat.com
>>     <mailto:marcandre.lureau@redhat.com>>
>>     >
>>     > According to g_tree_foreach() documentation:
>>     > "The tree may not be modified while iterating over it (you can't
>>     > add/remove items)."
>>
>>     Hum I did not see that.
>>     >
>>     > Fixes: 9a85e4b8f6 ("migration: Support gtree migration")
>>     > Cc: Eric Auger <eric.auger@redhat.com <mailto:eric.auger@redhat.com>>
>>     > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com
>>     <mailto:marcandre.lureau@redhat.com>>
>>     > ---
>>     >  tests/test-vmstate.c | 3 +--
>>     >  1 file changed, 1 insertion(+), 2 deletions(-)
>>     >
>>     > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
>>     > index f7b3868881..31aefa78f0 100644
>>     > --- a/tests/test-vmstate.c
>>     > +++ b/tests/test-vmstate.c
>>     > @@ -1078,7 +1078,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);
>>     it does not test the same thing I am afraid. If one of the trees
>>     contains more elements than the others this won't be detected.
>>
>>
>> -    assert(g_tree_nnodes(tree1) == 0);
>>
>> Was simply checking that all nodes from tree1 were deleted.
>>
>> Whatever else must have been checked elsewhere or differently by new code.
> compare_trees() iterates on tree1 and tries to find a fellow node in
> tree2 for each node in tree1. At least we need to check that the number
> of nodes is the same in both trees otherwise tree2 can have more nodes
> than tree1 and with the change this won't be detected.

Actually since we remove the nodes in tree2 when we find them,
assert(g_tree_nnodes(tree2) == 0) would detect if we have more nodes in
tree2 than in tree1.

On the opposite, if we have more nodes in tree1 than in tree2 the
original code failed to detect that case, - my fault -.
checking
assert(g_tree_nnodes(tree1) == g_tree_nnodes(tree2));
would fix the original algo I think.

Thanks

Eric

>>  
>>
>>     Also there is another case of removal inside the traversal in the
>>     match_node(): in match_interval_mapping_node() and match_domain_node()
>>
>>
>> Yes, but they don't update the traversed tree.
> Hum yes, you mean we exit the loop by returning true in the match
> function so that's OK.
> 
> So your modif + replacing
>     g_tree_foreach(tree1, diff_tree, &tp);
>     assert(g_tree_nnodes(tree1) == 0);
>     assert(g_tree_nnodes(tree2) == 0);
> 
> by
> 
> assert(g_tree_nnodes(tree1) == g_tree_nnodes(tree2));
> g_tree_foreach(tree1, diff_tree, &tp);

> 
> should do the job, no?


> 
> Thanks
> 
> Eric
> 
> 
>>
>>     Thanks
>>
>>     Eric
>>
>>
>>     >      return false;
>>     >  }
>>     > 
>>     > @@ -1088,7 +1087,7 @@ static void compare_trees(GTree *tree1,
>>     GTree *tree2,
>>     >      struct tree_cmp_data tp = {tree1, tree2, function};
>>     > 
>>     >      g_tree_foreach(tree1, diff_tree, &tp);
>>     > -    assert(g_tree_nnodes(tree1) == 0);> +   
>>     g_tree_destroy(g_tree_ref(tree1));
>>     >      assert(g_tree_nnodes(tree2) == 0);
>>     >  }
>>     > 
>>     >
>>
>>
>>
>>
>> -- 
>> Marc-André Lureau



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-08-28  7:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-27 16:18 [PATCH] test-vmstate: fix bad GTree usage, use-after-free marcandre.lureau
2020-08-27 16:28 ` Juan Quintela
2020-08-27 16:34 ` Auger Eric
2020-08-27 18:35   ` Marc-André Lureau
2020-08-28  7:19     ` Auger Eric
2020-08-28  7:52       ` Auger Eric

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).