[Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Bogdan Harjoc]

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

Git commit 8d3bc51 crashes on win32 on startup because qemu_tcg_init_vcpu
calls:

qemu_thread_create(th, qemu_tcg_cpu_thread_fn, ...
...
qemu_thread_get_handle(th)

which locks th->data->cs, a CRITICAL_SECTION which is initialized only in
the thread_fn, so it finds garbage.

Attached patch initializes it before calling _beginthreadex. GDB/windbg
probably start newly created threads sooner, because this doesn't happen
under a debugger.

With the patch below it boots until it crashes somewhere while attaching
disks (-hda raw_img).

"bt" in gdb only returns "#0  0x00000000 in ??" and generate-core-file
didn't work.

Cheers,

diff -du qemu-8d3bc51\qemu-thread-win32.c
qemu-8d3bc51-new\qemu-thread-win32.c
--- qemu-8d3bc51\qemu-thread-win32.c    Tue Dec 27 17:28:58 2011
+++ qemu-8d3bc51-new\qemu-thread-win32.c    Thu Dec 29 18:59:50 2011
@@ -215,8 +215,6 @@
     if (data->mode == QEMU_THREAD_DETACHED) {
         g_free(data);
         data = NULL;
-    } else {
-        InitializeCriticalSection(&data->cs);
     }
     TlsSetValue(qemu_thread_tls_index, data);
     qemu_thread_exit(start_routine(thread_arg));
@@ -287,6 +285,10 @@
     data->arg = arg;
     data->mode = mode;
     data->exited = false;
+
+    if (data->mode != QEMU_THREAD_DETACHED) {
+        InitializeCriticalSection(&data->cs);
+    }

     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
                                       data, 0, &thread->tid);

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

Re: [Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Paolo Bonzini]

On 12/29/2011 06:29 PM, Bogdan Harjoc wrote:
> Git commit 8d3bc51 crashes on win32 on startup because
> qemu_tcg_init_vcpu calls:
>
> qemu_thread_create(th, qemu_tcg_cpu_thread_fn, ...
> ...
> qemu_thread_get_handle(th)
>
> which locks th->data->cs, a CRITICAL_SECTION which is initialized only
> in the thread_fn, so it finds garbage.
>
> Attached patch initializes it before calling _beginthreadex. GDB/windbg
> probably start newly created threads sooner, because this doesn't happen
> under a debugger.
>
> With the patch below it boots until it crashes somewhere while attaching
> disks (-hda raw_img).
>
> "bt" in gdb only returns "#0  0x00000000 in ??" and generate-core-file
> didn't work.
>
> Cheers,
>
> diff -du qemu-8d3bc51\qemu-thread-win32.c
> qemu-8d3bc51-new\qemu-thread-win32.c
> --- qemu-8d3bc51\qemu-thread-win32.c    Tue Dec 27 17:28:58 2011
> +++ qemu-8d3bc51-new\qemu-thread-win32.c    Thu Dec 29 18:59:50 2011
> @@ -215,8 +215,6 @@
>       if (data->mode == QEMU_THREAD_DETACHED) {
>           g_free(data);
>           data = NULL;
> -    } else {
> -        InitializeCriticalSection(&data->cs);
>       }
>       TlsSetValue(qemu_thread_tls_index, data);
>       qemu_thread_exit(start_routine(thread_arg));
> @@ -287,6 +285,10 @@
>       data->arg = arg;
>       data->mode = mode;
>       data->exited = false;
> +
> +    if (data->mode != QEMU_THREAD_DETACHED) {
> +        InitializeCriticalSection(&data->cs);
> +    }
>
>       hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>                                         data, 0, &thread->tid);
>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Stefan Weil]

Am 29.12.2011 18:29, schrieb Bogdan Harjoc:
> Git commit 8d3bc51 crashes on win32 on startup because 
> qemu_tcg_init_vcpu calls:
>
> qemu_thread_create(th, qemu_tcg_cpu_thread_fn, ...
> ...
> qemu_thread_get_handle(th)
>
> which locks th->data->cs, a CRITICAL_SECTION which is initialized only 
> in the thread_fn, so it finds garbage.
>
> Attached patch initializes it before calling _beginthreadex. 
> GDB/windbg probably start newly created threads sooner, because this 
> doesn't happen under a debugger.
>
> With the patch below it boots until it crashes somewhere while 
> attaching disks (-hda raw_img).
>
> "bt" in gdb only returns "#0  0x00000000 in ??" and generate-core-file 
> didn't work.
>
> Cheers,
>
> diff -du qemu-8d3bc51\qemu-thread-win32.c 
> qemu-8d3bc51-new\qemu-thread-win32.c
> --- qemu-8d3bc51\qemu-thread-win32.c    Tue Dec 27 17:28:58 2011
> +++ qemu-8d3bc51-new\qemu-thread-win32.c    Thu Dec 29 18:59:50 2011
> @@ -215,8 +215,6 @@
>      if (data->mode == QEMU_THREAD_DETACHED) {
>          g_free(data);
>          data = NULL;
> -    } else {
> -        InitializeCriticalSection(&data->cs);
>      }
>      TlsSetValue(qemu_thread_tls_index, data);
>      qemu_thread_exit(start_routine(thread_arg));
> @@ -287,6 +285,10 @@
>      data->arg = arg;
>      data->mode = mode;
>      data->exited = false;
> +
> +    if (data->mode != QEMU_THREAD_DETACHED) {
> +        InitializeCriticalSection(&data->cs);
> +    }
>
>      hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>                                        data, 0, &thread->tid);
>

Hi,

could you please resend your patch with a Signed-by line?
And you should use "git format-patch" to create the patch.

See http://wiki.qemu.org/Contribute/SubmitAPatch for more information.

Thanks,

Stefan Weil

Re: [Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Stefan Weil]

Am 21.01.2012 23:08, schrieb Stefan Weil:
>
> Hi,
>
> could you please resend your patch with a Signed-by line?

Signed-off-by, of course. Sorry, it's too late for writing mails :-)


> And you should use "git format-patch" to create the patch.
>
> See http://wiki.qemu.org/Contribute/SubmitAPatch for more information.
>
> Thanks,
>
> Stefan Weil
>

Re: [Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Stefan Weil]

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

Am 29.12.2011 18:29, schrieb Bogdan Harjoc:
> Git commit 8d3bc51 crashes on win32 on startup because 
> qemu_tcg_init_vcpu calls:
>
> qemu_thread_create(th, qemu_tcg_cpu_thread_fn, ...
> ...
> qemu_thread_get_handle(th)
>
> which locks th->data->cs, a CRITICAL_SECTION which is initialized only 
> in the thread_fn, so it finds garbage.
>
> Attached patch initializes it before calling _beginthreadex. 
> GDB/windbg probably start newly created threads sooner, because this 
> doesn't happen under a debugger.
>
> With the patch below it boots until it crashes somewhere while 
> attaching disks (-hda raw_img).
>
> "bt" in gdb only returns "#0  0x00000000 in ??" and generate-core-file 
> didn't work.
>
> Cheers,
>
> diff -du qemu-8d3bc51\qemu-thread-win32.c 
> qemu-8d3bc51-new\qemu-thread-win32.c
> --- qemu-8d3bc51\qemu-thread-win32.c    Tue Dec 27 17:28:58 2011
> +++ qemu-8d3bc51-new\qemu-thread-win32.c    Thu Dec 29 18:59:50 2011
> @@ -215,8 +215,6 @@
>      if (data->mode == QEMU_THREAD_DETACHED) {
>          g_free(data);
>          data = NULL;
> -    } else {
> -        InitializeCriticalSection(&data->cs);
>      }
>      TlsSetValue(qemu_thread_tls_index, data);
>      qemu_thread_exit(start_routine(thread_arg));
> @@ -287,6 +285,10 @@
>      data->arg = arg;
>      data->mode = mode;
>      data->exited = false;
> +
> +    if (data->mode != QEMU_THREAD_DETACHED) {
> +        InitializeCriticalSection(&data->cs);
> +    }
>
>      hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>                                        data, 0, &thread->tid);
>


Tested-by: Stefan Weil <sw@weilnetz.de>


Hi Bogdan,

I can confirm that your patch fixes a crash which otherwise makes
QEMU unusable on Windows hosts.

Could you please sign your patch with a Signed-off-by line including
your name and e-mail address (similar to my Tested-by above)?
We need this before we can commit your patch to QEMU master.

See http://wiki.qemu.org/Contribute/SubmitAPatch for more information.

Please contact me if you need more information.

Regards,

Stefan


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

Re: [Qemu-devel] [PATCH] Init win32 CRITICAL_SECTION before starting thread; crash when attaching disks

[Sebastian Herbszt]

Stefan Weil wrote:
> Tested-by: Stefan Weil <sw@weilnetz.de>
>
>
> Hi Bogdan,
> 
> I can confirm that your patch fixes a crash which otherwise makes
> QEMU unusable on Windows hosts.

This patch likely fixes bug #922131 [1].

[1] https://bugs.launchpad.net/qemu/+bug/922131

Sebastian

[Qemu-devel] [PATCH] w32: Initialise critical section before starting thread (fix #922131)

[Stefan Weil]

This patch was contributed by Bogdan Harjoc. I added some assertions.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 qemu-thread-win32.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-thread-win32.c b/qemu-thread-win32.c
index fe9b931..3524c8b 100644
--- a/qemu-thread-win32.c
+++ b/qemu-thread-win32.c
@@ -215,8 +215,6 @@ static unsigned __stdcall win32_start_routine(void *arg)
     if (data->mode == QEMU_THREAD_DETACHED) {
         g_free(data);
         data = NULL;
-    } else {
-        InitializeCriticalSection(&data->cs);
     }
     TlsSetValue(qemu_thread_tls_index, data);
     qemu_thread_exit(start_routine(thread_arg));
@@ -227,6 +225,7 @@ void qemu_thread_exit(void *arg)
 {
     QemuThreadData *data = TlsGetValue(qemu_thread_tls_index);
     if (data) {
+        assert(data->mode != QEMU_THREAD_DETACHED);
         data->ret = arg;
         EnterCriticalSection(&data->cs);
         data->exited = true;
@@ -258,6 +257,7 @@ void *qemu_thread_join(QemuThread *thread)
         CloseHandle(handle);
     }
     ret = data->ret;
+    assert(data->mode != QEMU_THREAD_DETACHED);
     DeleteCriticalSection(&data->cs);
     g_free(data);
     return ret;
@@ -288,6 +288,10 @@ void qemu_thread_create(QemuThread *thread,
     data->mode = mode;
     data->exited = false;
 
+    if (data->mode != QEMU_THREAD_DETACHED) {
+        InitializeCriticalSection(&data->cs);
+    }
+
     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
                                       data, 0, &thread->tid);
     if (!hThread) {
@@ -314,6 +318,7 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
         return NULL;
     }
 
+    assert(data->mode != QEMU_THREAD_DETACHED);
     EnterCriticalSection(&data->cs);
     if (!data->exited) {
         handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH] w32: Initialise critical section before starting thread (fix #922131)

[Roy Tam]

Hi,

2012/1/31 Stefan Weil <sw@weilnetz.de>:
> This patch was contributed by Bogdan Harjoc. I added some assertions.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

[snip]

Thanks, it starts now but I hit another crash:
GNU gdb (GDB) 7.3
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from C:\msys\home\User\qemu\i386-softmmu/qemu-system-i386.exe...
done.
(gdb) r
Starting program:
C:\msys\home\User\qemu\i386-softmmu/qemu-system-i386.exe -L
..\\pc-bios -hda xp.vmdk
[New Thread 13020.0x32c4]
[New Thread 13020.0x2acc]
[New Thread 13020.0x2f74]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 13020.0x2f74]
0x7c81071e in SwitchToFiber () from C:\WINDOWS\system32\kernel32.dll
(gdb) bt
#0  0x7c81071e in SwitchToFiber () from C:\WINDOWS\system32\kernel32.dll
#1  0x0044790d in qemu_coroutine_switch (from_=0x1ab93fc, to_=0x1d4c5f0,
    action=COROUTINE_YIELD) at coroutine-win32.c:48
#2  0x00000001 in ?? ()
#3  0x004dc753 in coroutine_swap (from=0x1e00, to=0xff0a0000)
    at qemu-coroutine.c:31
#4  0x00411a39 in bdrv_rw_co (bs=0x1ab8008, sector_num=<optimized out>,
    buf=0x22a0000 "@", nb_sectors=1, is_write=false) at block.c:1335
#5  0x004887fc in ide_sector_read (s=0x1d1ffa8)
    at C:/msys/home/User/qemu/hw/ide/core.c:480
#6  0x0054eafa in memory_region_iorange_write (iorange=0x1d1f670, offset=7,
    width=1, data=150586501200084992) at C:/msys/home/User/qemu/memory.c:431
#7  0x00549981 in ioport_writeb_thunk (opaque=0x1d1f670, addr=7680, data=32)
    at C:/msys/home/User/qemu/ioport.c:211
#8  0x00549b7b in ioport_write (data=<optimized out>,
    address=<optimized out>, index=<optimized out>)
    at C:/msys/home/User/qemu/ioport.c:82
#9  cpu_outb (addr=503, val=0 '\000') at C:/msys/home/User/qemu/ioport.c:274
#10 0x02420397 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)