[PATCH v2] udevd: don't use alarm() for timeouts

[PATCH v2] udevd: don't use alarm() for timeouts

[Alan Jenkins]

alarm() is per-process; we can't use it for timeouts in a multi-threaded
udevd.  We need to explicitly track the total time spent waiting for
each event.

There is an issue here if the timeout expires in run_program().  If
run_program() returns without calling wait(), the process it forked
will become a zombie when it finally exits.  Currently, udev-event is
implemented as a process, so the zombie will be reparented to init and
reaped once the event is finished.  But that won't work for threads.

The solution is to fork twice:

udevd (event thread)
 udevd child process
  command

When the timeout expires, the event thread can return immediately.
The child process will stay blocked in wait(), until the command finally
finishes (or is killed).  We'll get a nice process tree showing that the
the hung process was started by udevd :-).

The child process passes the exit status of the command over a pipe.
This also provides a solution to the lack of a timeout in any of the
standard wait() system calls.  We can set a timeout on select() when
waiting on the pipe.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

---
v2: don't use the timeout from udev_device_get_timeout(), it's completely
    separate from the udev event timeout.

diff --git a/udev/test-udev.c b/udev/test-udev.c
index c6b8bf5..7e29b39 100644
--- a/udev/test-udev.c
+++ b/udev/test-udev.c
@@ -69,9 +69,6 @@ int main(int argc, char *argv[])
 	sigaction(SIGINT, &act, NULL);
 	sigaction(SIGTERM, &act, NULL);
 
-	/* trigger timeout to prevent hanging processes */
-	alarm(UDEV_EVENT_TIMEOUT);
-
 	action = getenv("ACTION");
 	devpath = getenv("DEVPATH");
 	subsystem = getenv("SUBSYSTEM");
@@ -98,10 +95,6 @@ int main(int argc, char *argv[])
 	event = udev_event_new(dev);
 	err = udev_event_execute_rules(event, rules);
 
-	/* rules may change/disable the timeout */
-	if (udev_device_get_event_timeout(dev) >= 0)
-		alarm(udev_device_get_event_timeout(dev));
-
 	if (err = 0 && !event->ignore_device && udev_get_run(udev))
 		udev_event_execute_run(event);
 
diff --git a/udev/udev-event.c b/udev/udev-event.c
index d521251..517471c 100644
--- a/udev/udev-event.c
+++ b/udev/udev-event.c
@@ -25,6 +25,7 @@
 #include <string.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>
+#include <sys/wait.h>
 #include <net/if.h>
 #include <linux/sockios.h>
 
@@ -41,6 +42,10 @@ struct udev_event *udev_event_new(struct udev_device *dev)
 	event->udev = udev_device_get_udev(dev);
 	udev_list_init(&event->run_list);
 	event->mode = 0660;
+
+	/* set timeout to prevent hanging processes */
+	event->timeout_usecs = UDEV_EVENT_TIMEOUT * USECS_PER_SECOND;
+
 	dbg(event->udev, "allocated event %p\n", event);
 	return event;
 }
@@ -520,7 +525,11 @@ static int rename_netif(struct udev_event *event)
 			}
 			dbg(event->udev, "wait for netif '%s' to become free, loop=%i\n",
 			    event->name, (90 * 20) - loop);
-			usleep(1000 * 1000 / 20);
+			usleep(USECS_PER_SECOND / 20);
+
+			event->timeout_usecs -= USECS_PER_SECOND / 20;
+			if (event->timeout_usecs <= 0)
+				break;
 		}
 	}
 exit:
@@ -559,6 +568,11 @@ int udev_event_execute_rules(struct udev_event *event, struct udev_rules *rules)
 		}
 
 		udev_rules_apply_to_event(rules, event);
+		if (event->timeout_usecs <= 0) {
+			err = -1;
+			goto exit_add;
+		}
+
 		if (event->tmp_node != NULL) {
 			dbg(event->udev, "cleanup temporary device node\n");
 			util_unlink_secure(event->udev, event->tmp_node);
@@ -636,6 +650,10 @@ exit_add:
 		udev_device_delete_db(dev);
 
 		udev_rules_apply_to_event(rules, event);
+		if (event->timeout_usecs <= 0) {
+			err = -1;
+			goto exit;
+		}
 		if (event->ignore_device) {
 			info(event->udev, "device event will be ignored\n");
 			goto exit;
@@ -692,6 +710,11 @@ exit_add:
 		}
 
 		udev_rules_apply_to_event(rules, event);
+		if (event->timeout_usecs <= 0) {
+			err = -1;
+			goto exit;
+		}
+
 		if (event->ignore_device) {
 			info(event->udev, "device event will be ignored\n");
 			goto exit;
@@ -708,6 +731,11 @@ exit_add:
 
 	/* default devices */
 	udev_rules_apply_to_event(rules, event);
+	if (event->timeout_usecs <= 0) {
+		err = -1;
+		goto exit;
+	}
+
 	if (event->ignore_device)
 		info(event->udev, "device event will be ignored\n");
 
@@ -738,7 +766,6 @@ int udev_event_execute_run(struct udev_event *event)
 			udev_monitor_unref(monitor);
 		} else {
 			char program[UTIL_PATH_SIZE];
-			char **envp;
 
 			udev_event_apply_format(event, cmd, program, sizeof(program));
 			if (event->trace)
@@ -746,8 +773,9 @@ int udev_event_execute_run(struct udev_event *event)
 				       udev_device_get_syspath(event->dev),
 				       udev_device_get_seqnum(event->dev),
 				       program);
-			envp = udev_device_get_properties_envp(event->dev);
-			if (util_run_program(event->udev, program, envp, NULL, 0, NULL) != 0) {
+			if (udev_event_run_program(event, program, NULL, 0, NULL) != 0) {
+				if (event->timeout_usecs <= 0)
+					return -1;
 				if (!udev_list_entry_get_flag(list_entry))
 					err = -1;
 			}
@@ -755,3 +783,292 @@ int udev_event_execute_run(struct udev_event *event)
 	}
 	return err;
 }
+
+int udev_event_run_program(struct udev_event *event, const char *command,
+			   char *result, size_t ressize, size_t *reslen)
+{
+	int status;
+	int outpipe[2] = {-1, -1};
+	int errpipe[2] = {-1, -1};
+	int statuspipe[2] = {-1, -1};
+	pid_t pid;
+	char **envp;
+	char arg[UTIL_PATH_SIZE];
+	char program[UTIL_PATH_SIZE];
+	char *argv[(sizeof(arg) / 2) + 1];
+	int devnull;
+	int i;
+	int err = 0;
+
+	envp = udev_device_get_properties_envp(event->dev);
+
+	/* build argv from command */
+	util_strscpy(arg, sizeof(arg), command);
+	i = 0;
+	if (strchr(arg, ' ') != NULL) {
+		char *pos = arg;
+
+		while (pos != NULL && pos[0] != '\0') {
+			if (pos[0] = '\'') {
+				/* do not separate quotes */
+				pos++;
+				argv[i] = strsep(&pos, "\'");
+				while (pos != NULL && pos[0] = ' ')
+					pos++;
+			} else {
+				argv[i] = strsep(&pos, " ");
+			}
+			dbg(event->udev, "arg[%i] '%s'\n", i, argv[i]);
+			i++;
+		}
+		argv[i] = NULL;
+	} else {
+		argv[0] = arg;
+		argv[1] = NULL;
+	}
+	info(event->udev, "'%s'\n", command);
+
+	/* prepare pipes from child to parent */
+	if (result != NULL || udev_get_log_priority(event->udev) >= LOG_INFO) {
+		if (pipe(outpipe) != 0) {
+			err(event->udev, "pipe failed: %m\n");
+			return -1;
+		}
+	}
+	if (udev_get_log_priority(event->udev) >= LOG_INFO) {
+		if (pipe(errpipe) != 0) {
+			err(event->udev, "pipe failed: %m\n");
+			return -1;
+		}
+	}
+	if (pipe(statuspipe) != 0) {
+		err(event->udev, "pipe failed: %m\n");
+		return -1;
+	}
+
+	/* allow programs in /lib/udev/ to be called without the path */
+	if (strchr(argv[0], '/') = NULL) {
+		util_strscpyl(program, sizeof(program), UDEV_PREFIX "/lib/udev/", argv[0], NULL);
+		argv[0] = program;
+	}
+
+	pid = fork();
+	switch(pid) {
+	case 0:
+		/* child closes parent ends of pipes */
+		if (outpipe[READ_END] > 0)
+			close(outpipe[READ_END]);
+		if (errpipe[READ_END] > 0)
+			close(errpipe[READ_END]);
+		close(statuspipe[READ_END]);
+
+		/* discard child output or connect to pipe */
+		devnull = open("/dev/null", O_RDWR);
+		if (devnull > 0) {
+			dup2(devnull, STDIN_FILENO);
+			if (outpipe[WRITE_END] < 0)
+				dup2(devnull, STDOUT_FILENO);
+			if (errpipe[WRITE_END] < 0)
+				dup2(devnull, STDERR_FILENO);
+			close(devnull);
+		} else
+			err(event->udev, "open /dev/null failed: %m\n");
+		if (outpipe[WRITE_END] > 0) {
+			dup2(outpipe[WRITE_END], STDOUT_FILENO);
+			close(outpipe[WRITE_END]);
+		}
+		if (errpipe[WRITE_END] > 0) {
+			dup2(errpipe[WRITE_END], STDERR_FILENO);
+			close(errpipe[WRITE_END]);
+		}
+
+		/* fork again, into command and minder process */
+		pid = vfork();
+		switch(pid) {
+		case 0: /* child: run command */
+			close(statuspipe[WRITE_END]);
+			execve(argv[0], argv, envp);
+			if (errno = ENOENT || errno = ENOTDIR) {
+				/* may be on a filesytem which is not mounted right now */
+				info(event->udev, "program '%s' not found\n", argv[0]);
+			} else {
+				/* other problems */
+				err(event->udev, "exec of program '%s' failed\n", argv[0]);
+			}
+			_exit(1);
+		case -1:
+			err(event->udev, "vfork of '%s' failed: %m\n", argv[0]);
+			_exit(1);
+		default: /* minder: wait for command to finish */
+			waitpid(pid, &status, 0);
+			write(statuspipe[WRITE_END], &status, sizeof(status));
+			_exit(0);
+		}
+	case -1:
+		err(event->udev, "fork of '%s' failed: %m\n", argv[0]);
+		return -1;
+	default:
+		/* read child */
+		{
+			struct timeval timeout;
+			size_t respos = 0;
+
+			/* parent closes child ends of pipes */
+			if (outpipe[WRITE_END] > 0)
+				close(outpipe[WRITE_END]);
+			if (errpipe[WRITE_END] > 0)
+				close(errpipe[WRITE_END]);
+			close(statuspipe[WRITE_END]);
+
+			timeout.tv_sec = event->timeout_usecs / USECS_PER_SECOND;
+			timeout.tv_usec = event->timeout_usecs % USECS_PER_SECOND;
+
+			/* read child output and exit status */
+			while (1) {
+				int fdcount;
+				int nfds = 0;
+				fd_set readfds;
+				ssize_t count;
+
+				FD_ZERO(&readfds);
+				if (outpipe[READ_END] > 0)
+					FD_SET(outpipe[READ_END], &readfds);
+				if (errpipe[READ_END] > 0)
+					FD_SET(errpipe[READ_END], &readfds);
+				if (statuspipe[READ_END] > 0)
+					FD_SET(statuspipe[READ_END], &readfds);
+
+				nfds = UDEV_MAX(nfds, outpipe[READ_END]);
+				nfds = UDEV_MAX(nfds, errpipe[READ_END]);
+				nfds = UDEV_MAX(nfds, statuspipe[READ_END]);
+
+				fdcount = select(nfds+1, &readfds, NULL, NULL, &timeout);
+				if (fdcount < 0) {
+					if (errno = EINTR)
+						continue;
+					err = -1;
+					break;
+				}
+				if (fdcount = 0) {
+					if (statuspipe[READ_END] > 0) {
+						err(event->udev, "failed waiting for '%s'\n", argv[0]);
+						err = -1;
+						event->timeout_usecs = 0;
+						close(statuspipe[READ_END]);
+					}
+					break;
+				}
+
+				/* get stdout */
+				if (outpipe[READ_END] > 0 && FD_ISSET(outpipe[READ_END], &readfds)) {
+					char inbuf[1024];
+					char *pos;
+					char *line;
+
+					count = read(outpipe[READ_END], inbuf, sizeof(inbuf)-1);
+					if (count <= 0) {
+						close(outpipe[READ_END]);
+						outpipe[READ_END] = -1;
+						if (count < 0) {
+							err(event->udev, "stdin read failed: %m\n");
+							err = -1;
+						}
+						continue;
+					}
+					inbuf[count] = '\0';
+
+					/* store result for rule processing */
+					if (result) {
+						if (respos + count < ressize) {
+							memcpy(&result[respos], inbuf, count);
+							respos += count;
+						} else {
+							err(event->udev, "ressize %ld too short\n", (long)ressize);
+							err = -1;
+						}
+					}
+					pos = inbuf;
+
+					while ((line = strsep(&pos, "\n")))
+						if (pos || line[0] != '\0')
+							info(event->udev, "'%s' (stdout) '%s'\n", argv[0], line);
+				}
+
+				/* get stderr */
+				if (errpipe[READ_END] > 0 && FD_ISSET(errpipe[READ_END], &readfds)) {
+					char errbuf[1024];
+					char *pos;
+					char *line;
+
+					count = read(errpipe[READ_END], errbuf, sizeof(errbuf)-1);
+					if (count <= 0) {
+						close(errpipe[READ_END]);
+						errpipe[READ_END] = -1;
+						if (count < 0)
+							err(event->udev, "stderr read failed: %m\n");
+						continue;
+					}
+					errbuf[count] = '\0';
+					pos = errbuf;
+
+					while ((line = strsep(&pos, "\n")))
+						if (pos || line[0] != '\0')
+							info(event->udev, "'%s' (stderr) '%s'\n", argv[0], line);
+				}
+
+				/* get exit status */
+				if (statuspipe[READ_END] > 0 && FD_ISSET(statuspipe[READ_END], &readfds)) {
+					/* get exit status of minder */
+					waitpid(pid, &status, 0);
+					if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
+						/* vfork failure - already logged */
+						err = -1;
+						break;
+					}
+
+					/* read exit status of command */
+					count = read(statuspipe[READ_END], &status, sizeof(status));
+					if (count < 0) {
+						err(event->udev, "exit status read failed: %m\n");
+						err = -1;
+						break;
+					}
+					close(statuspipe[READ_END]);
+					statuspipe[READ_END] = -1;
+
+					if (WIFEXITED(status)) {
+						status = WEXITSTATUS(status);
+						info(event->udev, "'%s' returned with status %i\n", argv[0], status);
+						if (status != 0)
+							err = -1;
+					} else {
+						err(event->udev, "'%s' abnormal exit\n", command);
+						err = -1;
+					}
+
+					event->timeout_usecs = timeout.tv_sec * USECS_PER_SECOND + timeout.tv_usec;
+
+					/* Continue reading in case data remains, but don't block. */
+					timeout.tv_sec = 0;
+					timeout.tv_usec = 0;
+				}
+			}
+
+			/* return the childs stdout string */
+			if (result) {
+				result[respos] = '\0';
+				dbg(event->udev, "result='%s'\n", result);
+				if (reslen)
+					*reslen = respos;
+			}
+		}
+		if (outpipe[READ_END] > 0)
+			close(outpipe[READ_END]);
+		if (errpipe[READ_END] > 0)
+			close(errpipe[READ_END]);
+		if (statuspipe[READ_END] > 0)
+			close(statuspipe[READ_END]);
+	}
+
+	return err;
+}
diff --git a/udev/udev-rules.c b/udev/udev-rules.c
index e6452a6..cde8a39 100644
--- a/udev/udev-rules.c
+++ b/udev/udev-rules.c
@@ -730,16 +730,13 @@ static int import_file_into_properties(struct udev_device *dev, const char *file
 	return 0;
 }
 
-static int import_program_into_properties(struct udev_device *dev, const char *program)
+static int import_program_into_properties(struct udev_event *event, const char *program)
 {
-	struct udev *udev = udev_device_get_udev(dev);
-	char **envp;
 	char result[4096];
 	size_t reslen;
 	char *line;
 
-	envp = udev_device_get_properties_envp(dev);
-	if (util_run_program(udev, program, envp, result, sizeof(result), &reslen) != 0)
+	if (udev_event_run_program(event, program, result, sizeof(result), &reslen) != 0)
 		return -1;
 
 	line = result;
@@ -751,7 +748,7 @@ static int import_program_into_properties(struct udev_device *dev, const char *p
 			pos[0] = '\0';
 			pos = &pos[1];
 		}
-		import_property_from_string(dev, line);
+		import_property_from_string(event->dev, line);
 		line = pos;
 	}
 	return 0;
@@ -784,10 +781,11 @@ static int import_parent_into_properties(struct udev_device *dev, const char *fi
 	return 0;
 }
 
-#define WAIT_LOOP_PER_SECOND		50
-static int wait_for_file(struct udev_device *dev, const char *file, int timeout)
+#define WAIT_LOOP_PER_SECOND	50
+#define WAIT_LOOP_USECS		(USECS_PER_SECOND / WAIT_LOOP_PER_SECOND)
+
+static int wait_for_file(struct udev_event *event, const char *file, int timeout)
 {
-	struct udev *udev = udev_device_get_udev(dev);
 	char filepath[UTIL_PATH_SIZE];
 	char devicepath[UTIL_PATH_SIZE];
 	struct stat stats;
@@ -797,27 +795,30 @@ static int wait_for_file(struct udev_device *dev, const char *file, int timeout)
 	devicepath[0] = '\0';
 	if (file[0] != '/') {
 		util_strscpyl(devicepath, sizeof(devicepath),
-			      udev_get_sys_path(udev), udev_device_get_devpath(dev), NULL);
+			      udev_get_sys_path(event->udev), udev_device_get_devpath(event->dev), NULL);
 		util_strscpyl(filepath, sizeof(filepath), devicepath, "/", file, NULL);
 		file = filepath;
 	}
 
-	dbg(udev, "will wait %i sec for '%s'\n", timeout, file);
+	dbg(event->udev, "waiting for '%s'\n", file);
 	while (--loop) {
 		/* lookup file */
 		if (stat(file, &stats) = 0) {
-			info(udev, "file '%s' appeared after %i loops\n", file, (timeout * WAIT_LOOP_PER_SECOND) - loop-1);
+			info(event->udev, "file '%s' appeared after %i loops\n", file, (timeout * WAIT_LOOP_PER_SECOND) - loop-1);
 			return 0;
 		}
 		/* make sure, the device did not disappear in the meantime */
 		if (devicepath[0] != '\0' && stat(devicepath, &stats) != 0) {
-			info(udev, "device disappeared while waiting for '%s'\n", file);
+			info(event->udev, "device disappeared while waiting for '%s'\n", file);
 			return -2;
 		}
-		info(udev, "wait for '%s' for %i mseconds\n", file, 1000 / WAIT_LOOP_PER_SECOND);
-		usleep(1000 * 1000 / WAIT_LOOP_PER_SECOND);
+		info(event->udev, "wait for '%s' for %i mseconds\n", file, 1000 / WAIT_LOOP_PER_SECOND);
+		usleep(WAIT_LOOP_USECS);
+		event->timeout_usecs -= WAIT_LOOP_USECS;
+		if (event->timeout_usecs < 0)
+			break;
 	}
-	info(udev, "waiting for '%s' failed\n", file);
+	info(event->udev, "waiting for '%s' failed\n", file);
 	return -1;
 }
 
@@ -2041,7 +2042,7 @@ int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event
 				int found;
 
 				udev_event_apply_format(event, &rules->buf[cur->key.value_off], filename, sizeof(filename));
-				found = (wait_for_file(event->dev, filename, 10) = 0);
+				found = (wait_for_file(event, filename, 10) = 0);
 				if (!found && (cur->key.op != OP_NOMATCH))
 					goto nomatch;
 				break;
@@ -2140,18 +2141,18 @@ int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event
 		case TK_M_PROGRAM:
 			{
 				char program[UTIL_PATH_SIZE];
-				char **envp;
 				char result[UTIL_PATH_SIZE];
 
 				free(event->program_result);
 				event->program_result = NULL;
 				udev_event_apply_format(event, &rules->buf[cur->key.value_off], program, sizeof(program));
-				envp = udev_device_get_properties_envp(event->dev);
 				info(event->udev, "PROGRAM '%s' %s:%u\n",
 				     program,
 				     &rules->buf[rule->rule.filename_off],
 				     rule->rule.filename_line);
-				if (util_run_program(event->udev, program, envp, result, sizeof(result), NULL) != 0) {
+				if (udev_event_run_program(event, program, result, sizeof(result), NULL) != 0) {
+					if (event->timeout_usecs <= 0)
+						return -1;
 					if (cur->key.op != OP_NOMATCH)
 						goto nomatch;
 				} else {
@@ -2189,9 +2190,12 @@ int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event
 				     import,
 				     &rules->buf[rule->rule.filename_off],
 				     rule->rule.filename_line);
-				if (import_program_into_properties(event->dev, import) != 0)
+				if (import_program_into_properties(event, import) != 0) {
+					if (event->timeout_usecs <= 0)
+						return -1;
 					if (cur->key.op != OP_NOMATCH)
 						goto nomatch;
+				}
 				break;
 			}
 		case TK_M_IMPORT_PARENT:
diff --git a/udev/udev-util.c b/udev/udev-util.c
index 645293d..10a5adf 100644
--- a/udev/udev-util.c
+++ b/udev/udev-util.c
@@ -238,214 +238,3 @@ int util_resolve_subsys_kernel(struct udev *udev, const char *string,
 	udev_device_unref(dev);
 	return 0;
 }
-
-int util_run_program(struct udev *udev, const char *command, char **envp,
-		     char *result, size_t ressize, size_t *reslen)
-{
-	int status;
-	int outpipe[2] = {-1, -1};
-	int errpipe[2] = {-1, -1};
-	pid_t pid;
-	char arg[UTIL_PATH_SIZE];
-	char program[UTIL_PATH_SIZE];
-	char *argv[(sizeof(arg) / 2) + 1];
-	int devnull;
-	int i;
-	int err = 0;
-
-	/* build argv from command */
-	util_strscpy(arg, sizeof(arg), command);
-	i = 0;
-	if (strchr(arg, ' ') != NULL) {
-		char *pos = arg;
-
-		while (pos != NULL && pos[0] != '\0') {
-			if (pos[0] = '\'') {
-				/* do not separate quotes */
-				pos++;
-				argv[i] = strsep(&pos, "\'");
-				while (pos != NULL && pos[0] = ' ')
-					pos++;
-			} else {
-				argv[i] = strsep(&pos, " ");
-			}
-			dbg(udev, "arg[%i] '%s'\n", i, argv[i]);
-			i++;
-		}
-		argv[i] = NULL;
-	} else {
-		argv[0] = arg;
-		argv[1] = NULL;
-	}
-	info(udev, "'%s'\n", command);
-
-	/* prepare pipes from child to parent */
-	if (result != NULL || udev_get_log_priority(udev) >= LOG_INFO) {
-		if (pipe(outpipe) != 0) {
-			err(udev, "pipe failed: %m\n");
-			return -1;
-		}
-	}
-	if (udev_get_log_priority(udev) >= LOG_INFO) {
-		if (pipe(errpipe) != 0) {
-			err(udev, "pipe failed: %m\n");
-			return -1;
-		}
-	}
-
-	/* allow programs in /lib/udev/ to be called without the path */
-	if (argv[0][0] != '/') {
-		util_strscpyl(program, sizeof(program), UDEV_PREFIX "/lib/udev/", argv[0], NULL);
-		argv[0] = program;
-	}
-
-	pid = fork();
-	switch(pid) {
-	case 0:
-		/* child closes parent ends of pipes */
-		if (outpipe[READ_END] > 0)
-			close(outpipe[READ_END]);
-		if (errpipe[READ_END] > 0)
-			close(errpipe[READ_END]);
-
-		/* discard child output or connect to pipe */
-		devnull = open("/dev/null", O_RDWR);
-		if (devnull > 0) {
-			dup2(devnull, STDIN_FILENO);
-			if (outpipe[WRITE_END] < 0)
-				dup2(devnull, STDOUT_FILENO);
-			if (errpipe[WRITE_END] < 0)
-				dup2(devnull, STDERR_FILENO);
-			close(devnull);
-		} else
-			err(udev, "open /dev/null failed: %m\n");
-		if (outpipe[WRITE_END] > 0) {
-			dup2(outpipe[WRITE_END], STDOUT_FILENO);
-			close(outpipe[WRITE_END]);
-		}
-		if (errpipe[WRITE_END] > 0) {
-			dup2(errpipe[WRITE_END], STDERR_FILENO);
-			close(errpipe[WRITE_END]);
-		}
-		execve(argv[0], argv, envp);
-		if (errno = ENOENT || errno = ENOTDIR) {
-			/* may be on a filesytem which is not mounted right now */
-			info(udev, "program '%s' not found\n", argv[0]);
-		} else {
-			/* other problems */
-			err(udev, "exec of program '%s' failed\n", argv[0]);
-		}
-		_exit(1);
-	case -1:
-		err(udev, "fork of '%s' failed: %m\n", argv[0]);
-		return -1;
-	default:
-		/* read from child if requested */
-		if (outpipe[READ_END] > 0 || errpipe[READ_END] > 0) {
-			ssize_t count;
-			size_t respos = 0;
-
-			/* parent closes child ends of pipes */
-			if (outpipe[WRITE_END] > 0)
-				close(outpipe[WRITE_END]);
-			if (errpipe[WRITE_END] > 0)
-				close(errpipe[WRITE_END]);
-
-			/* read child output */
-			while (outpipe[READ_END] > 0 || errpipe[READ_END] > 0) {
-				int fdcount;
-				fd_set readfds;
-
-				FD_ZERO(&readfds);
-				if (outpipe[READ_END] > 0)
-					FD_SET(outpipe[READ_END], &readfds);
-				if (errpipe[READ_END] > 0)
-					FD_SET(errpipe[READ_END], &readfds);
-				fdcount = select(UDEV_MAX(outpipe[READ_END], errpipe[READ_END])+1, &readfds, NULL, NULL, NULL);
-				if (fdcount < 0) {
-					if (errno = EINTR)
-						continue;
-					err = -1;
-					break;
-				}
-
-				/* get stdout */
-				if (outpipe[READ_END] > 0 && FD_ISSET(outpipe[READ_END], &readfds)) {
-					char inbuf[1024];
-					char *pos;
-					char *line;
-
-					count = read(outpipe[READ_END], inbuf, sizeof(inbuf)-1);
-					if (count <= 0) {
-						close(outpipe[READ_END]);
-						outpipe[READ_END] = -1;
-						if (count < 0) {
-							err(udev, "stdin read failed: %m\n");
-							err = -1;
-						}
-						continue;
-					}
-					inbuf[count] = '\0';
-
-					/* store result for rule processing */
-					if (result) {
-						if (respos + count < ressize) {
-							memcpy(&result[respos], inbuf, count);
-							respos += count;
-						} else {
-							err(udev, "ressize %ld too short\n", (long)ressize);
-							err = -1;
-						}
-					}
-					pos = inbuf;
-					while ((line = strsep(&pos, "\n")))
-						if (pos || line[0] != '\0')
-							info(udev, "'%s' (stdout) '%s'\n", argv[0], line);
-				}
-
-				/* get stderr */
-				if (errpipe[READ_END] > 0 && FD_ISSET(errpipe[READ_END], &readfds)) {
-					char errbuf[1024];
-					char *pos;
-					char *line;
-
-					count = read(errpipe[READ_END], errbuf, sizeof(errbuf)-1);
-					if (count <= 0) {
-						close(errpipe[READ_END]);
-						errpipe[READ_END] = -1;
-						if (count < 0)
-							err(udev, "stderr read failed: %m\n");
-						continue;
-					}
-					errbuf[count] = '\0';
-					pos = errbuf;
-					while ((line = strsep(&pos, "\n")))
-						if (pos || line[0] != '\0')
-							info(udev, "'%s' (stderr) '%s'\n", argv[0], line);
-				}
-			}
-			if (outpipe[READ_END] > 0)
-				close(outpipe[READ_END]);
-			if (errpipe[READ_END] > 0)
-				close(errpipe[READ_END]);
-
-			/* return the childs stdout string */
-			if (result) {
-				result[respos] = '\0';
-				dbg(udev, "result='%s'\n", result);
-				if (reslen)
-					*reslen = respos;
-			}
-		}
-		waitpid(pid, &status, 0);
-		if (WIFEXITED(status)) {
-			info(udev, "'%s' returned with status %i\n", argv[0], WEXITSTATUS(status));
-			if (WEXITSTATUS(status) != 0)
-				err = -1;
-		} else {
-			err(udev, "'%s' abnormal exit\n", command);
-			err = -1;
-		}
-	}
-	return err;
-}
diff --git a/udev/udev.h b/udev/udev.h
index 8f2c1c6..00e6fa3 100644
--- a/udev/udev.h
+++ b/udev/udev.h
@@ -28,6 +28,7 @@
 
 #define DEFAULT_FAKE_PARTITIONS_COUNT		15
 #define UDEV_EVENT_TIMEOUT			180
+#define USECS_PER_SECOND			(1000 * 1000)
 
 #define UDEV_CTRL_SOCK_PATH			"@" UDEV_PREFIX "/org/kernel/udev/udevd"
 
@@ -64,6 +65,7 @@ struct udev_event {
 	uid_t uid;
 	gid_t gid;
 	struct udev_list_node run_list;
+	int timeout_usecs;
 	pid_t pid;
 	int exitstatus;
 	time_t queue_time;
@@ -99,6 +101,9 @@ int udev_event_execute_run(struct udev_event *event);
 size_t udev_event_apply_format(struct udev_event *event, const char *src, char *dest, size_t size);
 int udev_event_apply_subsys_kernel(struct udev_event *event, const char *string,
 				   char *result, size_t maxsize, int read_value);
+int udev_event_run_program(struct udev_event *event, const char *command,
+			   char *result, size_t ressize, size_t *reslen);
+
 
 /* udev-watch.c */
 extern int inotify_fd;
@@ -120,8 +125,6 @@ int util_delete_path(struct udev *udev, const char *path);
 int util_unlink_secure(struct udev *udev, const char *filename);
 uid_t util_lookup_user(struct udev *udev, const char *user);
 gid_t util_lookup_group(struct udev *udev, const char *group);
-int util_run_program(struct udev *udev, const char *command, char **envp,
-		     char *result, size_t ressize, size_t *reslen);
 int util_resolve_subsys_kernel(struct udev *udev, const char *string,
 				      char *result, size_t maxsize, int read_value);
 
diff --git a/udev/udevd.c b/udev/udevd.c
index 5ee61d2..6e613d2 100644
--- a/udev/udevd.c
+++ b/udev/udevd.c
@@ -155,16 +155,9 @@ static void event_fork(struct udev_event *event)
 		sigaction(SIGCHLD, &act, NULL);
 		sigaction(SIGHUP, &act, NULL);
 
-		/* set timeout to prevent hanging processes */
-		alarm(UDEV_EVENT_TIMEOUT);
-
 		/* apply rules, create node, symlinks */
 		err = udev_event_execute_rules(event, rules);
 
-		/* rules may change/disable the timeout */
-		if (udev_device_get_event_timeout(event->dev) >= 0)
-			alarm(udev_device_get_event_timeout(event->dev));
-
 		/* execute RUN= */
 		if (err = 0 && !event->ignore_device && udev_get_run(event->udev))
 			udev_event_execute_run(event);

Re: [PATCH v2] udevd: don't use alarm() for timeouts

[Kay Sievers]

On Tue, May 26, 2009 at 12:31, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> alarm() is per-process; we can't use it for timeouts in a multi-threaded
> udevd.  We need to explicitly track the total time spent waiting for
> each event.
>
> There is an issue here if the timeout expires in run_program().  If
> run_program() returns without calling wait(), the process it forked
> will become a zombie when it finally exits.  Currently, udev-event is
> implemented as a process, so the zombie will be reparented to init and
> reaped once the event is finished.  But that won't work for threads.
>
> The solution is to fork twice:
>
> udevd (event thread)
>  udevd child process
>  command
>
> When the timeout expires, the event thread can return immediately.
> The child process will stay blocked in wait(), until the command finally
> finishes (or is killed).  We'll get a nice process tree showing that the
> the hung process was started by udevd :-).

The event runs all programs serialized, one after the other, can't we
just kill the program that does not return in time, and wait for it to
cleanup the process, instead of just exiting the event process?

Thanks,
Kay

Re: [PATCH v2] udevd: don't use alarm() for timeouts

[Alan Jenkins]

Kay Sievers wrote:
> On Tue, May 26, 2009 at 12:31, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>   
>> alarm() is per-process; we can't use it for timeouts in a multi-threaded
>> udevd.  We need to explicitly track the total time spent waiting for
>> each event.
>>
>> There is an issue here if the timeout expires in run_program().  If
>> run_program() returns without calling wait(), the process it forked
>> will become a zombie when it finally exits.  Currently, udev-event is
>> implemented as a process, so the zombie will be reparented to init and
>> reaped once the event is finished.  But that won't work for threads.
>>
>> The solution is to fork twice:
>>
>> udevd (event thread)
>>  udevd child process
>>  command
>>
>> When the timeout expires, the event thread can return immediately.
>> The child process will stay blocked in wait(), until the command finally
>> finishes (or is killed).  We'll get a nice process tree showing that the
>> the hung process was started by udevd :-).
>>     
>
> The event runs all programs serialized, one after the other, can't we
> just kill the program that does not return in time, and wait for it to
> cleanup the process, instead of just exiting the event process?
>   

That makes sense if you also change the timeout model - from per-event
timeouts, to per-command timeouts.

The second fork is cheap, because it's a vfork.  But if you like
per-command timeouts, I would be happy to see it go away.

I'm not sure about killing.  Do we need to escalate to SIGKILL?  Do we
e.g. allow half the timeout before sending SIGTERM, then another half
before sending SIGKILL?  The process could even be unkillable - is it ok
to block after SIGKILL, or do we need another timeout?

I could keep a list of timed out processes instead, and reap them on
SIGCHLD.  Would that be better?

There are other workarounds for the lack of a timeout in sys_wait(), so
I don't think that's a problem.  We can require that commands close
stdout & stderr pipes when they exit - i.e. do not pass them on to a
long-running child.  (At the moment there's a debian script "net.agent"
which has to do this for debug mode - it would need fixing to always do it).

Alan

Re: [PATCH v2] udevd: don't use alarm() for timeouts

[Kay Sievers]

On Tue, May 26, 2009 at 20:05, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:

>> The event runs all programs serialized, one after the other, can't we
>> just kill the program that does not return in time, and wait for it to
>> cleanup the process, instead of just exiting the event process?
>
> That makes sense if you also change the timeout model - from per-event
> timeouts, to per-command timeouts.

We can still track how long we a running and use that timeout when
waiting for the actual program to finish, right?

> The second fork is cheap, because it's a vfork.  But if you like
> per-command timeouts, I would be happy to see it go away.

I guess, it's simpler to understand and vfork() is something that even
POSIX removed from the spec. :)

> I'm not sure about killing.  Do we need to escalate to SIGKILL?  Do we
> e.g. allow half the timeout before sending SIGTERM, then another half
> before sending SIGKILL?

I guess a few seconds would be good enough before we try to finally KILL it.

> The process could even be unkillable - is it ok
> to block after SIGKILL, or do we need another timeout?

Hmm, yeah, maybe we just block.

I don't remember any problems with the timeouts, they are mainly there
to prevent bad things that make udev operations unreliable like people
running apps with user interfaces from udev rules, which we see every
other month. So I guess killing would be the right thing to do anyway.
:)

> I could keep a list of timed out processes instead, and reap them on
> SIGCHLD.  Would that be better?

You mean as an alternative to blocking the event process?

> There are other workarounds for the lack of a timeout in sys_wait(), so
> I don't think that's a problem.  We can require that commands close
> stdout & stderr pipes when they exit - i.e. do not pass them on to a
> long-running child.  (At the moment there's a debian script "net.agent"
> which has to do this for debug mode - it would need fixing to always do it).

I don't think we can really assume anything from called programs. :)

Thanks,
Kay

Re: [PATCH v2] udevd: don't use alarm() for timeouts

[Alan Jenkins]

Kay Sievers wrote:

<Snip.  Apologies for snipping all your questions. I think it's become a
little confused and this is the most important issue.>

[Could run_program() avoid forking twice in a threaded udevd, and kill
timed-out commands so they don't become zombies when they eventually
finish?]

> On Tue, May 26, 2009 at 20:05, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>   
>> There are other workarounds for the lack of a timeout in sys_wait(), so
>> I don't think that's a problem.  We can require that commands close
>> stdout & stderr pipes when they exit - i.e. do not pass them on to a
>> long-running child.  (At the moment there's a debian script "net.agent"
>> which has to do this for debug mode - it would need fixing to always do it).
>>     
>
> I don't think we can really assume anything from called programs. :)
>
> Thanks,
> Kay
>   

Sounds like the voice of experience.

The main constraint I'm working around is the lack of a timeout in
sys_wait*().  Forking twice allows the parent process to block on a pipe
instead, and use a select() timeout.

If we don't want to fork twice, and we can't rely on EOF on the
command's stdout pipe, I think the only alternative is to wait for
SIGCHLD.  The signal handler would check for finished PIDs with WNOHANG,
and look them up in a list to see which thread needs waking up.  In that
case, it's not essential to kill timed out commands.  The signal handler
can reap them just as easily as it reaps a command which is being waited
for by an event.

Forking twice is simpler though.  It's more direct, and it also easily
works with threading disabled.  At the moment my thread-specific code is
reasonably contained.  I have a ./configure option which switches back
to using separate processes, in case pthreads support is missing or
broken on some platforms.

Thanks
Alan