Skip to contentSkip to conversation

Moving alarm_timer assignment before atexit()

PATCH In review Amos Kong 3 participants 5 messages Aug 6, 2013 – Aug 8, 2013

lore
Share & tools
lore

Amos Kong

author
#0 lore raw

PATCH 1/1 Moving alarm_timer assignment before atexit()

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 <[email protected]>
---
 qemu-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
qemu-timer.c +1 -1
index b2d95e2..9490105 100644
@@ -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:
Signature · 1 line
1.8.3.1
#1 lore raw
Quoted 4 lines · 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.

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

Quoted 26 lines

So move alarm_timer
assignment before atexit().

Signed-off-by Amos Kong <[email protected]>
---
 qemu-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
qemu-timer.c +1 -1
index b2d95e2..9490105 100644
@@ -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:
Reviewed-by
#2 lore raw
Quoted 8 lines · Laszlo Ersek
Quoted 4 lines · 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.

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 <[email protected]>
author
#3 lore raw
Quoted 9 lines · Stefan Hajnoczi
Quoted 8 lines · Laszlo Ersek
Quoted 4 lines · 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.

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;

Quoted 7 lines

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.

Quoted 1 line
Reviewed-by Stefan Hajnoczi <[email protected]>

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) ?

qemu-timer.c +0 -0
index b2d95e2..023e4ae 100644
@@ -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

Signature · 1 line
			Amos.
#4 lore raw
Quoted 10 lines · Amos Kong

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) ?

qemu-timer.c +0 -0
index b2d95e2..023e4ae 100644
@@ -728,8 +728,10 @@ static void win32_rearm_timer(struct
 qemu_alarm_timer *t,

qemu-timer.c static void win32_rearm_timer(struct · line 736

Quoted 10 lines
qemu-timer.c +4 -2
 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