[BusyBox] [gemorin at debian.org: Bug#212764: 2 race conditions in init implementation]

Bastian Blank waldi at debian.org
Fri Sep 26 06:50:35 UTC 2003


----- Forwarded message from Guillaume Morin <gemorin at debian.org> -----

Subject: Bug#212764: 2 race conditions in init implementation
Date: Thu, 25 Sep 2003 17:41:58 -0400
From: Guillaume Morin <gemorin at debian.org>
To: Debian Bug Tracking System <submit at bugs.debian.org>

Package: busybox-cvs
Version: 0.60.99.cvs20030819-3 (not installed)
Severity: important
Tags: patch

Hi,

I have found two races conditions in the run() function of busybox's
init implementation (static pid_t run(const struct init_action *a)).

The runtime behavior is that the call to wait in waitfor() blocks
forever. I can easily reproduce it on SMP system. Actually, it should
only easily reproducable on 2 processors systems.

- 1st race condition

around line 450
        sigemptyset(&nmask);
        sigaddset(&nmask, SIGCHLD);
        sigprocmask(SIG_BLOCK, &nmask, &omask);

        if ((pid = fork()) == 0) {
		/* exec the command here  and exit*/
	}

	sigprocmask(SIG_SETMASK, &omask, NULL);
	return pid;

	/* the parent will call wait() after returning */

There is a race here. If the child exits before the SIGCHLD is
unblocked, wait() will block forever.

I have fixed the bug by replacing the wait() call in waitfor() with a
waitpid() call and handle the error.

- 2nd race condition

on line 486
                        /* Now fork off another process to just hang around */
                        if ((pid = fork()) < 0) {
                                message(LOG | CONSOLE, "Can't fork!");
                                _exit(1);
                        }

                        if (pid > 0) {

                                /* We are the parent -- wait till the child is done */
                                signal(SIGINT, SIG_IGN);
                                signal(SIGTSTP, SIG_IGN);
                                signal(SIGQUIT, SIG_IGN);
                                signal(SIGCHLD, SIG_DFL);

                                /* Wait for child to exit */
                                while ((tmp_pid = waitpid(pid, &junk, 0)) != pid);

If the child exits before the call to signal(SIGCHLD, SIG_DFL), another
sigchld handler might be executed or the signal may be ignored. In both
cases, waitpid() won't work, so this condition should be tested. I have
modified that in my patch.

There is another possible fix. You can move signal(SIGCHLD, SIG_DFL)
before the fork on line 457 ... Your call, this may be cleaner. The
signal calls are still very racy but it should not really matter.

--- /home/guillaum/tmp/burp/busybox-cvs-0.60.99.cvs20030819/init/init.c 2003-08-19 11:59:13.000000000 +0000
+++ busybox-cvs-0.60.99.cvs20030819/init/init.c 2003-09-25 21:24:41.000000000 +0000
@@ -496,7 +496,11 @@
                                signal(SIGCHLD, SIG_DFL);

                                /* Wait for child to exit */
-                               while ((tmp_pid = waitpid(pid, &junk, 0)) != pid);
+                               while ((tmp_pid = waitpid(pid, &junk, 0)) != pid) {
+                                       if (tmp_pid == -1 && errno == ECHILD)
+                                               break;
+                                       // FIXME handle other errors
+                               }

                                /* See if stealing the controlling tty back is necessary */
                                pgrp = tcgetpgrp(0);
@@ -620,12 +624,14 @@

        pid = run(a);
        while (1) {
-               wpid = wait(&status);
-               if (wpid > 0 && wpid != pid) {
-                       continue;
-               }
+               wpid = waitpid(pid,&status,0);
                if (wpid == pid)
                        break;
+               if (wpid == -1 && errno == ECHILD)
+                       /* we missed its termination */
+                       break;
+               /* FIXME other errors should maybe trigger an error, but allow
+                * the program to continue */
        }
        return wpid;
 }


HTH.

Guillaume.

----- End forwarded message -----

Bastian

-- 
Four thousand throats may be cut in one night by a running man.
		-- Klingon Soldier, "Day of the Dove", stardate unknown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
Url : http://lists.busybox.net/pipermail/busybox/attachments/20030926/fece58ea/attachment-0002.pgp 


More information about the busybox mailing list