Re: [PULL 1/2] test-char: abort on serial test error

[Eduardo Habkost]

On Tue, Dec 15, 2020 at 02:29:22PM -0500, Eduardo Habkost wrote:
> On Sun, Dec 13, 2020 at 11:51:05PM +0100, Philippe Mathieu-Daudé wrote:
> > On 7/28/20 4:31 PM, Marc-André Lureau wrote:
> > > We are having issues debugging and bisecting this issue that happen
> > > mostly on patchew. Let's make it abort where it failed to gather some
> > > new informations.
> > 
> > "good" news, this started to fail on Gitlab (centos7):
> > 
> > Running test test-char
> > Unexpected error in object_property_try_add() at ../qom/object.c:1220:
> > attempt to add duplicate property 'serial-id' to object (type 'container')
> > ERROR test-char - too few tests run (expected 38, got 9)
> > make: *** [run-test-86] Error 1
> > 
> > https://gitlab.com/philmd/qemu/-/jobs/908114388
> > https://gitlab.com/philmd/qemu/-/jobs/908114389
> > https://gitlab.com/philmd/qemu/-/jobs/908114390
> 
> Do we have any clue what could be causing this?  After looking at
> the code, it smells like memory corruption.
> 
> At first, I thought it could be due to the multi-threaded test
> cases, but the test binary seems to be crashing before the
> multi-threaded test cases have an opportunity to run.

I think I know what's happening:

- char_file_test_internal() creates chr using qemu_chardev_new().
- qemu_chardev_new() automatically assigns ID, adds
  chardev to the QOM tree.
- char_file_test_internal() does _not_ own the reference
  to the created object.
- char_file_test_internal() incorrectly calls object_unref().
- object is freed but, but /containers now has a dangling
  pointer.
- char_serial_test() creates a chardev with ID "serial-id", and
  it ends up being allocated at the same address as the old
  object.
- char_serial_test() correctly calls object_unparent().
- object_property_del_child() looks for the right child property
  in the hashtable, finds the dangling pointer with the same
  address, removes the wrong property, leaves a dangling
  "serial-id" property.
- New object is created by char_serial_test() with ID "serial-id".
- object_property_try_add_child() will fail because of the
  dangling "serial-id" property.

The bug in char_file_test_internal() was detected using the
following patch.

I believe the bug was introduced by commit 1e419ee68fa5
("chardev: generate an internal id when none given") because it
changed the reference ownership semantics of
qemu_chardev_new(NULL, ...).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/qom/object.c b/qom/object.c
index f2ae6e6b2a..5cfed6d7c6 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -685,6 +685,7 @@ static void object_finalize(void *data)
     object_deinit(obj, ti);
 
     g_assert(obj->ref == 0);
+    g_assert(obj->parent == NULL);
     if (obj->free) {
         obj->free(obj);
     }

-- 
Eduardo