[PULL 0/2] Update slirp (+ debug test-serial)

[PULL 0/2] Update slirp (+ debug test-serial)

[Marc-André Lureau]

The following changes since commit 264991512193ee50e27d43e66f832d5041cf3b28:

  Merge remote-tracking branch 'remotes/ericb/tags/pull-bitmaps-2020-07-27' into staging (2020-07-28 14:38:17 +0100)

are available in the Git repository at:

  https://github.com/elmarco/qemu.git tags/slirp-pull-request

for you to fetch changes up to 9c15f57891af7c2cb3baf2d66a1b1f3f87a665ba:

  slirp: update to latest stable-4.2 branch (2020-07-28 18:27:59 +0400)

----------------------------------------------------------------
slirp: update to latest stable-4.2 branch

----------------------------------------------------------------

Marc-André Lureau (2):
  test-char: abort on serial test error
  slirp: update to latest stable-4.2 branch

 tests/test-char.c | 2 +-
 slirp             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.28.0.rc2.1.g3d20111cbd

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

[Marc-André Lureau]

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.

Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-char.c b/tests/test-char.c
index 614bdac2df..d35cc839bc 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -1200,7 +1200,7 @@ static void char_serial_test(void)
 
     /* test tty alias */
     qemu_opt_set(opts, "backend", "tty", &error_abort);
-    chr = qemu_chr_new_from_opts(opts, NULL, NULL);
+    chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
     g_assert_nonnull(chr);
     object_unparent(OBJECT(chr));
 
-- 
2.28.0.rc2.1.g3d20111cbd

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

[Philippe Mathieu-Daudé]

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

> 
> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 614bdac2df..d35cc839bc 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1200,7 +1200,7 @@ static void char_serial_test(void)
>  
>      /* test tty alias */
>      qemu_opt_set(opts, "backend", "tty", &error_abort);
> -    chr = qemu_chr_new_from_opts(opts, NULL, NULL);
> +    chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
>      g_assert_nonnull(chr);
>      object_unparent(OBJECT(chr));
>  
> 

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

[Eduardo Habkost]

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.

> 
> > 
> > Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/test-char.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/test-char.c b/tests/test-char.c
> > index 614bdac2df..d35cc839bc 100644
> > --- a/tests/test-char.c
> > +++ b/tests/test-char.c
> > @@ -1200,7 +1200,7 @@ static void char_serial_test(void)
> >  
> >      /* test tty alias */
> >      qemu_opt_set(opts, "backend", "tty", &error_abort);
> > -    chr = qemu_chr_new_from_opts(opts, NULL, NULL);
> > +    chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
> >      g_assert_nonnull(chr);
> >      object_unparent(OBJECT(chr));
> >  
> > 
> 
> 

-- 
Eduardo

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

[PULL 2/2] slirp: update to latest stable-4.2 branch

[Marc-André Lureau]

Dr. David Alan Gilbert (1):
      ip_stripoptions use memmove

Jindrich Novy (4):
      Fix possible infinite loops and use-after-free
      Use secure string copy to avoid overflow
      Be sure to initialize sockaddr structure
      Check lseek() for failure

Marc-André Lureau (2):
      util: do not silently truncate
      Merge branch 'stable-4.2' into 'stable-4.2'

Philippe Mathieu-Daudé (3):
      Fix win32 builds by using the SLIRP_PACKED definition
      Fix constness warnings
      Remove unnecessary break

Ralf Haferkamp (2):
      Drop bogus IPv6 messages
      Fix MTU check

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp b/slirp
index 2faae0f778..ce94eba204 160000
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 2faae0f778f818fadc873308f983289df697eb93
+Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
-- 
2.28.0.rc2.1.g3d20111cbd

Re: [PULL 0/2] Update slirp (+ debug test-serial)

[Peter Maydell]

On Tue, 28 Jul 2020 at 15:31, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> The following changes since commit 264991512193ee50e27d43e66f832d5041cf3b28:
>
>   Merge remote-tracking branch 'remotes/ericb/tags/pull-bitmaps-2020-07-27' into staging (2020-07-28 14:38:17 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/elmarco/qemu.git tags/slirp-pull-request
>
> for you to fetch changes up to 9c15f57891af7c2cb3baf2d66a1b1f3f87a665ba:
>
>   slirp: update to latest stable-4.2 branch (2020-07-28 18:27:59 +0400)
>
> ----------------------------------------------------------------
> slirp: update to latest stable-4.2 branch
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM