[Qemu-devel] [PATCH] Moving alarm_timer assignment before atexit()

[Qemu-devel] [PATCH] Moving alarm_timer assignment before atexit()

[Amos Kong]

We register exit clean function by atexit(),
but alarm_timer is NULL here. If exit is caused
between atexit() and alarm_timer assignment,
real timer can't be cleaned. So move alarm_timer
assignment before atexit().

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index b2d95e2..9490105 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -767,11 +767,11 @@ int init_timer_alarm(void)
         goto fail;
     }
 
+    alarm_timer = t;
     atexit(quit_timers);
 #ifdef CONFIG_POSIX
     pthread_atfork(NULL, NULL, reinit_timers);
 #endif
-    alarm_timer = t;
     return 0;
 
 fail:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] Moving alarm_timer assignment before atexit()

[Laszlo Ersek]

On 08/07/13 01:29, Amos Kong wrote:
> We register exit clean function by atexit(),
> but alarm_timer is NULL here. If exit is caused
> between atexit() and alarm_timer assignment,
> real timer can't be cleaned.

That's correct in general, but I don't see how it could happen in the
code being patched. pthread_atfork() won't call exit().

Thanks,
Laszlo

> So move alarm_timer
> assignment before atexit().
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu-timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index b2d95e2..9490105 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -767,11 +767,11 @@ int init_timer_alarm(void)
>          goto fail;
>      }
>  
> +    alarm_timer = t;
>      atexit(quit_timers);
>  #ifdef CONFIG_POSIX
>      pthread_atfork(NULL, NULL, reinit_timers);
>  #endif
> -    alarm_timer = t;
>      return 0;
>  
>  fail:
> 

Re: [Qemu-devel] [PATCH] Moving alarm_timer assignment before atexit()

[Stefan Hajnoczi]

On Wed, Aug 07, 2013 at 08:39:19AM +0200, Laszlo Ersek wrote:
> On 08/07/13 01:29, Amos Kong wrote:
> > We register exit clean function by atexit(),
> > but alarm_timer is NULL here. If exit is caused
> > between atexit() and alarm_timer assignment,
> > real timer can't be cleaned.
> 
> That's correct in general, but I don't see how it could happen in the
> code being patched. pthread_atfork() won't call exit().

Agreed.  I can remember thinking about this when reading the code and
deciding not to bother changing it.

Since the patch is on the list though, we might as well apply it.

The only thing I suggest changing is to note that this is currently not
a bug, just a clean-up.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH] Moving alarm_timer assignment before atexit()

[Amos Kong]

On Wed, Aug 07, 2013 at 09:57:19AM +0200, Stefan Hajnoczi wrote:
> On Wed, Aug 07, 2013 at 08:39:19AM +0200, Laszlo Ersek wrote:
> > On 08/07/13 01:29, Amos Kong wrote:
> > > We register exit clean function by atexit(),
> > > but alarm_timer is NULL here. If exit is caused
> > > between atexit() and alarm_timer assignment,
> > > real timer can't be cleaned.
> > 
> > That's correct in general, but I don't see how it could happen in the
> > code being patched. pthread_atfork() won't call exit().

I try to sleep 10 seconds after atexit(), but no crash occurred when I
killed qemu process.

atexit(quit_timer);
sleep(10);            // kill qemu by 'pkill qemu', no crash
pthread_atfork();
alarm_timer = t;

> Agreed.  I can remember thinking about this when reading the code and
> deciding not to bother changing it.
> 
> Since the patch is on the list though, we might as well apply it.
> 
> The only thing I suggest changing is to note that this is currently not
> a bug, just a clean-up.

It seems just a cleanup.
 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>


BTW, can we add a check in quit_timers() to avoid one kind of crash
(try to quit the uninited timers, or quit timer repeatedly) ?


diff --git a/qemu-timer.c b/qemu-timer.c
index b2d95e2..023e4ae 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -728,8 +728,10 @@ static void win32_rearm_timer(struct
qemu_alarm_timer *t,
 static void quit_timers(void)
 {
     struct qemu_alarm_timer *t = alarm_timer;
-    alarm_timer = NULL;
-    t->stop(t);
+    if (t) {
+        alarm_timer = NULL;
+        t->stop(t);
+    }
 }
 
 #ifdef CONFIG_POSIX

-- 
			Amos.

Re: [Qemu-devel] [PATCH] Moving alarm_timer assignment before atexit()

[Stefan Hajnoczi]

On Wed, Aug 07, 2013 at 04:17:29PM +0800, Amos Kong wrote:
> BTW, can we add a check in quit_timers() to avoid one kind of crash
> (try to quit the uninited timers, or quit timer repeatedly) ?
> 
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index b2d95e2..023e4ae 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -728,8 +728,10 @@ static void win32_rearm_timer(struct
> qemu_alarm_timer *t,
>  static void quit_timers(void)
>  {
>      struct qemu_alarm_timer *t = alarm_timer;
> -    alarm_timer = NULL;
> -    t->stop(t);
> +    if (t) {
> +        alarm_timer = NULL;
> +        t->stop(t);
> +    }
>  }

This is unnecessary once your other patch has been applied since t will
be initialized before quit_timers() is installed.

If we do ever hit the problem it should be debugged because assumptions
about the timer lifecycle must be broken.  So avoiding the segfault
isn't really useful here, I think.

Stefan