On Thu, 2010-03-18 at 01:00 -0400, Laine Stump wrote:
The original run_program simply called system(3), which was
expedient,
but failed to do things like close open file descriptors prior to
exec'ing the child binary. This particular omission caused complaints
when SELinux audited for leaked file descriptors during exec.
The new run program does the fork/exec manually, and in between it
closes all open file descriptors in the child process, as well as
clearing and restoring the signal mask to prevent a potential race
condition when killing off the parent's signal handlers in the child
process.
The code was heavily inspired by virRun() in libvirt, but is greatly
simplified, since netcf doesn't (currently) need all the facilities of
virRun().
---
configure.ac | 7 +++
src/netcf.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 133 insertions(+), 13 deletions(-)
ACK, good stuff. Some minor nits:
diff --git a/src/netcf.c b/src/netcf.c
index 7cfcc6a..7c642a3 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -27,6 +27,10 @@
#include <string.h>
#include <stdarg.h>
#include <stdio.h>
+#include <unistd.h>
+#include <sys/wait.h>
+#include <signal.h>
+#include <errno.h>
#include "safe-alloc.h"
#include "internal.h"
@@ -262,25 +266,134 @@ int ncf_put_aug(struct netcf *ncf, const char *aug_xml, char
**ncf_xml) {
* Internal helpers
*/
+static int
+exec_program(struct netcf *ncf,
+ const char *const*argv,
+ const char *commandline,
+ pid_t *pid)
+{
+ sigset_t oldmask, newmask;
+ struct sigaction sig_action;
+ char errbuf[128];
+
+ /* commandline is only used for error reporting */
+ if (commandline == NULL)
+ commandline = argv[0];
+ /*
+ * Need to block signals now, so that child process can safely
+ * kill off caller's signal handlers without a race.
+ */
+ sigfillset(&newmask);
+ if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) {
+ report_error(ncf, NETCF_EEXEC,
+ "failed to set signal mask while forking for '%s':
%s",
+ commandline, strerror_r(errno, errbuf, sizeof(errbuf)));
+ goto error;
+ }
+
+ *pid = fork();
+
+ ERR_THROW(*pid < 0, ncf, EEXEC,
+ "failed to set signal mask while forking for '%s': %s",
+ commandline, strerror_r(errno, errbuf, sizeof(errbuf)));
The error message seems like a cut&paste error; should just be 'failed
to fork'
+
+ execvp(argv[0], (char **) argv);
+
+ /* if execvp() returns, it has failed */
+ report_error(ncf, NETCF_EEXEC, "Failed to execute '%s': %s",
+ commandline, strerror_r(errno, errbuf, sizeof(errbuf)));
There's not really any point in reporting the error here, since nobody's
going to ever look at this struct netcf (in the child) You're also
catching this in the parent with the waitpid.
+ _exit(1);
+
+error:
+ /* This is cleanup of parent process only - child
+ should never jump here on error */
+ return -1;
+}
+
+/**
+ * Run a command without using the shell.
+ *
+ * return 0 if the command run and exited with 0 status; Otherwise
+ * return -1
+ *
+ */
int run_program(struct netcf *ncf, const char *const *argv) {
- int status, success;
- char *argv_str = argv_to_string(argv);
+ pid_t childpid;
+ int exitstatus, waitret;
+ char *argv_str;
int ret = -1;
+ char errbuf[128];
- /* BIG FIXME!!! Before any general release, this *must* be
- * replaced with a call to a function similar to libVirt's
- * virRun(), and if there is an error returned, anything the
- * program produced on stderr or stdout should be placed in
- * ncf->errdetails.
- */
- status = system(argv_str);
- success = WIFEXITED(status) && WEXITSTATUS(status) == 0;
- ERR_THROW(!success, ncf, EEXEC,
- "Running '%s' failed with error code %d",
- argv_str == NULL ? argv[0] : argv_str, WEXITSTATUS(status));
+ argv_str = argv_to_string(argv);
+ ERR_NOMEM(argv_str == NULL, ncf);
+ exec_program(ncf, argv, argv_str, &childpid);
+ ERR_BAIL(ncf);
+
+ while ((waitret = waitpid(childpid, &exitstatus, 0) == -1) &&
+ errno == EINTR) {
+ /* empty loop */
+ }
+
+ ERR_THROW(waitret == -1, ncf, EEXEC,
+ "Failed waiting for completion of '%s': %s",
+ argv_str, strerror_r(errno, errbuf, sizeof(errbuf)));
+ ERR_THROW(!WIFEXITED(exitstatus) && WIFSIGNALED(exitstatus), ncf, EEXEC,
+ "'%s' exited abnormally with signal: %d",
+ argv_str, WTERMSIG(exitstatus));
"terminated by signal %d" might be more accurate - the whole point of
this error is that the child did _not_ call exit() in any guise.
+ ERR_THROW(!WIFEXITED(exitstatus), ncf, EEXEC,
+ "'%s' exited improperly: %d",
+ argv_str, WEXITSTATUS(exitstatus));
Same here - better talk about 'termination' than 'exit' to avoid any
confusion.
David