[pacman-dev] [PATCH 1/2] redirect scriptlet stderr synchronously through alpm

[Jonathan Conder]

Fixes FS#18770, and hopefully an occasional deadlock in my frontend as well.
For simplicity it redirects all scriptlet output through SCRIPTLET_INFO, and
all callbacks in the child process have been replaced for thread-safety.
Also adds a shell to the fake root set up for pactests, which was not needed
previously due to a bug (debian #582847) in fakechroot.

Signed-off-by: Jonathan Conder <j at skurvy.no-ip.org>
---
 lib/libalpm/util.c    |   91 +++++++++++++++++++++++++++++-------------------
 test/pacman/pmtest.py |    5 ++-
 2 files changed, 59 insertions(+), 37 deletions(-)

I noticed the list has been quiet for a while, so I thought this would be a good
time to resubmit. Only change here is the location of the pactest, which git
moved automagically :). I am nearly certain now that this will fix the deadlock
in my frontend (for details see the RATIONALE section of man pthread_atfork).

diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
index ffebe9e..46460f0 100644
--- a/lib/libalpm/util.c
+++ b/lib/libalpm/util.c
@@ -456,6 +456,7 @@ int _alpm_run_chroot(const char *root, const char *cmd)
 {
 	char cwd[PATH_MAX];
 	pid_t pid;
+	int pipefd[2];
 	int restore_cwd = 0;
 	int retval = 0;
 
@@ -479,6 +480,12 @@ int _alpm_run_chroot(const char *root, const char *cmd)
 	/* Flush open fds before fork() to avoid cloning buffers */
 	fflush(NULL);
 
+	if(pipe(pipefd) == -1) {
+		_alpm_log(PM_LOG_ERROR, _("could not create pipe (%s)\n"), strerror(errno));
+		retval = 1;
+		goto cleanup;
+	}
+
 	/* fork- parent and child each have seperate code blocks below */
 	pid = fork();
 	if(pid == -1) {
@@ -488,55 +495,67 @@ int _alpm_run_chroot(const char *root, const char *cmd)
 	}
 
 	if(pid == 0) {
-		FILE *pipe;
 		/* this code runs for the child only (the actual chroot/exec) */
-		_alpm_log(PM_LOG_DEBUG, "chrooting in %s\n", root);
+		close(1);
+		close(2);
+		while(dup2(pipefd[1], 1) == -1 && errno == EINTR);
+		while(dup2(pipefd[1], 2) == -1 && errno == EINTR);
+		close(pipefd[0]);
+		close(pipefd[1]);
+
+		/* use fprintf instead of _alpm_log to send output through the parent */
 		if(chroot(root) != 0) {
-			_alpm_log(PM_LOG_ERROR, _("could not change the root directory (%s)\n"),
-					strerror(errno));
+			fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno));
 			exit(1);
 		}
 		if(chdir("/") != 0) {
-			_alpm_log(PM_LOG_ERROR, _("could not change directory to / (%s)\n"),
-					strerror(errno));
+			fprintf(stderr, _("could not change directory to / (%s)\n"), strerror(errno));
 			exit(1);
 		}
 		umask(0022);
-		pipe = popen(cmd, "r");
-		if(!pipe) {
-			_alpm_log(PM_LOG_ERROR, _("call to popen failed (%s)\n"),
-					strerror(errno));
-			exit(1);
-		}
-		while(!feof(pipe)) {
-			char line[PATH_MAX];
-			if(fgets(line, PATH_MAX, pipe) == NULL)
-				break;
-			alpm_logaction("%s", line);
-			EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL);
-		}
-		retval = pclose(pipe);
-		exit(WEXITSTATUS(retval));
+		execl("/bin/sh", "sh", "-c", cmd, (char *) NULL);
+		fprintf(stderr, _("call to execl failed (%s)\n"), strerror(errno));
+		exit(1);
 	} else {
 		/* this code runs for the parent only (wait on the child) */
-		pid_t retpid;
 		int status;
-		do {
-			retpid = waitpid(pid, &status, 0);
-		} while(retpid == -1 && errno == EINTR);
-		if(retpid == -1) {
-			_alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"),
-			          strerror(errno));
+		FILE *pipe;
+
+		close(pipefd[1]);
+		pipe = fdopen(pipefd[0], "r");
+		if(pipe == NULL) {
+			close(pipefd[0]);
 			retval = 1;
-			goto cleanup;
 		} else {
-			/* check the return status, make sure it is 0 (success) */
-			if(WIFEXITED(status)) {
-				_alpm_log(PM_LOG_DEBUG, "call to waitpid succeeded\n");
-				if(WEXITSTATUS(status) != 0) {
-					_alpm_log(PM_LOG_ERROR, _("command failed to execute correctly\n"));
-					retval = 1;
-				}
+			while(!feof(pipe)) {
+				char line[PATH_MAX];
+				if(fgets(line, PATH_MAX, pipe) == NULL)
+					break;
+				alpm_logaction("%s", line);
+				EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL);
+			}
+			fclose(pipe);
+		}
+
+		while(waitpid(pid, &status, 0) == -1) {
+			if(errno != EINTR) {
+				_alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"), strerror(errno));
+				retval = 1;
+				goto cleanup;
+			}
+		}
+
+		/* report error from above after the child has exited */
+		if(retval != 0) {
+			_alpm_log(PM_LOG_ERROR, _("could not open pipe (%s)\n"), strerror(errno));
+			goto cleanup;
+		}
+		/* check the return status, make sure it is 0 (success) */
+		if(WIFEXITED(status)) {
+			_alpm_log(PM_LOG_DEBUG, "call to waitpid succeeded\n");
+			if(WEXITSTATUS(status) != 0) {
+				_alpm_log(PM_LOG_ERROR, _("command failed to execute correctly\n"));
+				retval = 1;
 			}
 		}
 	}
diff --git a/test/pacman/pmtest.py b/test/pacman/pmtest.py
index b1d778e..5c8881c 100755
--- a/test/pacman/pmtest.py
+++ b/test/pacman/pmtest.py
@@ -110,10 +110,13 @@ def generate(self):
         tmpdir = os.path.join(self.root, TMPDIR)
         logdir = os.path.join(self.root, os.path.dirname(LOGFILE))
         etcdir = os.path.join(self.root, os.path.dirname(PACCONF))
-        for dir in [dbdir, cachedir, syncdir, tmpdir, logdir, etcdir]:
+        bindir = os.path.join(self.root, "bin")
+        for dir in [dbdir, cachedir, syncdir, tmpdir, logdir, etcdir, bindir]:
             if not os.path.isdir(dir):
                 vprint("\t%s" % dir[len(self.root)+1:])
                 os.makedirs(dir, 0755)
+        # Only the dynamically linked binary is needed for fakechroot
+        shutil.copy("/bin/sh", bindir)
 
         # Configuration file
         vprint("    Creating configuration file")
-- 
1.7.2.1