Date: Mon, 22 Nov 2010 23:08:10 -0800 From: Garrett Cooper <yanegomi@gmail.com> To: bug-followup@FreeBSD.org, peter@trumanbrewery.com Cc: Gavin Atkinson <gavin@freebsd.org>, freebsd-sysinstall@freebsd.org Subject: Re: bin/38854: sysinstall(8): resetting during setup causes the target installation path to change from "/mnt" (new root partition) to "/" (the memory disk) Message-ID: <AANLkTim5bHwuVVHJk5jPWvULJ7ZKRnutO-u7HP4bE-%2B9@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Hi,
I prototyped up a solution for this issue by banking on the fact
that the *fork(2) system calls actually do a copy on write of several
process variables, including the current working directory, etc.
Example:
$ ./test_forking_and_chdiring
[0] getwd = /tmp
[1] getwd = /usr/home/gcooper
[2] getwd = /tmp
[3] getwd = /usr/home/gcooper
So at least now sysinstall is starting from a sane state
consistently, instead of just reexecuting itself via system(3) with
-fakeInit and -restart (ugh). I was concerned about sysinstall closing
file descriptors on exec, etc, but this appears to be mostly baseless.
Cleaning up these dead options should be done in later commits.
I executed sysinstall from the command line in multiuser given the
following 4 scenarios:
1. Exit (exit code was 0).
2. ^C:
a. Continue (process continued as usual).
b. Restart (process `restarted' by continuing on in the loop
again -- it was rather amusing when I discovered that it reexecutes
itself and can essentially overload the stack eventually if it gets
into an infinite loop :/).
c. Abort (process exited with 1 as I designed it to).
There might be some corner cases with usage, but this focuses on
the primary areas. I was hoping to use vfork(2) (because it would have
made the logic a bit simpler, but unfortunately vfork isn't isolated
enough to do proper signal handling with SIGINT (I think that the
parent process and the child process share the same signal vectors,
but I could be wrong... I should look).
Ideally sysinstall should be run from a custom /etc/rc script that
sets up all of these variables and ensures that the sysinstall process
is playing by the rules, but that's something else that needs to be
resolved down the line for this particular issue. This is just a quick
and dirty stopgap fix.
Credit goes to Gavin for pointing me to this annoying problem :).
Thanks,
-Garrett
#include <sys/types.h>
#include <sys/wait.h>
#include <err.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
int
main(void)
{
int status;
switch (fork()) {
case -1:
err(1, "fork");
case 0:
if (chdir("/tmp") == -1)
err(1, "chroot");
printf("[0] getwd = %s\n", getwd(NULL));
exit(0);
default:
if (wait(&status) == -1)
err(1, "wait");
else if (status)
errx(1, "exit status != 0");
}
printf("[1] getwd = %s\n", getwd(NULL));
switch (vfork()) {
case -1:
err(1, "vfork");
case 0:
if (chdir("/tmp") == -1)
err(1, "chroot");
printf("[2] getwd = %s\n", getwd(NULL));
exit(0);
default:
if (wait(&status) == -1)
err(1, "wait");
else if (status)
errx(1, "exit status != 0");
}
printf("[3] getwd = %s\n", getwd(NULL));
exit(0);
}
[-- Attachment #2 --]
Index: usr.sbin/sysinstall/system.c
===================================================================
--- usr.sbin/sysinstall/system.c (revision 215661)
+++ usr.sbin/sysinstall/system.c (working copy)
@@ -58,24 +58,10 @@
static int
intr_restart(dialogMenuItem *self)
{
- int ret, fd, fdmax;
- char *arg;
-
mediaClose();
free_variables();
- fdmax = getdtablesize();
- for (fd = 3; fd < fdmax; fd++)
- close(fd);
-
- if (RunningAsInit)
- arg = "-restart -fakeInit";
- else
- arg = "-restart";
-
- ret = execl(StartName, StartName, arg, NULL);
- msgDebug("execl failed (%s)\n", strerror(errno));
- /* NOTREACHED */
- return -1;
+ /* We'll return back to main from here. */
+ _exit(1);
}
static dialogMenuItem intrmenu[] = {
Index: usr.sbin/sysinstall/main.c
===================================================================
--- usr.sbin/sysinstall/main.c (revision 215661)
+++ usr.sbin/sysinstall/main.c (working copy)
@@ -35,10 +35,11 @@
*/
#include "sysinstall.h"
-#include <sys/signal.h>
#include <sys/fcntl.h>
+#include <sys/resource.h>
#include <sys/time.h>
-#include <sys/resource.h>
+#include <err.h>
+#include <signal.h>
const char *StartName; /* Initial contents of argv[0] */
const char *ProgName = "sysinstall";
@@ -57,8 +58,9 @@
char titlestr[80], *arch, *osrel, *ostype;
struct rlimit rlim;
char *arg;
- int i;
+ int exit_status, i;
int optionArgs = 0;
+ pid_t rv, sysinstall_proper;
/* Record name to be able to restart */
StartName = argv[0];
@@ -88,8 +90,73 @@
}
if (getpid() == 1)
- RunningAsInit = TRUE;
-
+ RunningAsInit = TRUE;
+
+ /* We don't work too well when running as non-root anymore */
+ if (geteuid() != 0) {
+ fprintf(stderr, "Error: This utility should only be run as root.\n");
+ return 1;
+ }
+
+ /*
+ * To simplify things dealing with cleanup as sysinstall is reentrant
+ * (sadly), it's just easier to fork a new process once we know that
+ * we're root and running as init.
+ *
+ * Please do not move this logic.
+ */
+ do {
+
+ /* Reset the process group just in case. */
+ setpgid(0, getpid());
+
+ sysinstall_proper = fork();
+
+ switch (sysinstall_proper) {
+ case -1:
+ err(1, "fork failed");
+ case 0:
+ break;
+ default:
+
+ /* Let the child take the heat. */
+ signal(SIGINT, SIG_IGN);
+ setpgrp(0, sysinstall_proper);
+
+ do {
+ rv = waitpid(sysinstall_proper, &exit_status, 0);
+ if (errno == EINTR)
+ /* Be kind to the CPU */
+ sleep(1);
+ } while (rv == -1 && errno == EINTR);
+
+ if (rv == -1) {
+ /*
+ * Just in case (see ECHILD/SA_NOCLDWAIT error under wait(2).
+ * errant sysinstall processes => bad.
+ */
+ kill(9, sysinstall_proper);
+ err(1, "waiting for sysinstall proper failed");
+ } else if (exit_status != 0) {
+ /* For developer debug. */
+#if 0
+ if (WIFSIGNALED(exit_status)) {
+ errx(1, "sysinstall proper signaled with signal = %d",
+ WTERMSIG(exit_status));
+ } else if (WIFEXITED(exit_status)) {
+ errx(1, "sysinstall proper exited with exit code = %d",
+ WEXITSTATUS(exit_status));
+ }
+#endif
+ exit(1);
+ } else
+ /* All's good! */
+ exit(0);
+ break;
+ }
+
+ } while (sysinstall_proper != 0);
+
/* Catch fatal signals and complain about them if running as init */
if (RunningAsInit) {
signal(SIGBUS, screech);
@@ -97,12 +164,6 @@
}
signal(SIGPIPE, SIG_IGN);
- /* We don't work too well when running as non-root anymore */
- if (geteuid() != 0) {
- fprintf(stderr, "Error: This utility should only be run as root.\n");
- return 1;
- }
-
/*
* Given what it does sysinstall (and stuff sysinstall runs like
* pkg_add) shouldn't be subject to process limits. Better to just
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTim5bHwuVVHJk5jPWvULJ7ZKRnutO-u7HP4bE-%2B9>