"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> The LXC patches identified a race condition between fork/exec'ing child
> processes and signal handlers.
Looks fine modulo a few details:
> diff -r 1dbfb08d365d src/util.c
...
> @@ -104,9 +109,23 @@
> _virExec(virConnectPtr conn,
> const char *const*argv,
> int *retpid, int infd, int *outfd, int *errfd, int non_block) {
> - int pid, null;
> + int pid, null, i;
Use pid_t as the type for "pid".
Hmm... it'd be good to do the same to preexisting *retpid, (above) too.
> int pipeout[2] = {-1,-1};
> int pipeerr[2] = {-1,-1};
> + sigset_t oldmask, newmask;
> + struct sigaction sig_action;
> +
> + /*
> + * 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) {
This should test "!= 0", since the function is specified to return
"nonzero" upon failure. Two more uses below.
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("cannot block signals: %s"),
> + strerror(errno));
> + return -1;
> + }
>
> if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) {
> ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> @@ -122,6 +141,34 @@
> goto cleanup;
> }
>
> + if (outfd) {
> + if(non_block &&
I know this is moved code, but you might want to take the opportunity
to add a space between the "if" and the following open parenthesis.
> + virSetNonBlock(pipeout[0]) == -1) {
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("Failed to set non-blocking file descriptor flag"));
> + goto cleanup;
> + }
> +
> + if(virSetCloseExec(pipeout[0]) == -1) {
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("Failed to set close-on-exec file descriptor flag"));
> + goto cleanup;
> + }
> + }
> + if (errfd) {
> + if(non_block &&
> + virSetNonBlock(pipeerr[0]) == -1) {
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("Failed to set non-blocking file descriptor flag"));
> + goto cleanup;
> + }
> + if(virSetCloseExec(pipeerr[0]) == -1) {
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("Failed to set close-on-exec file descriptor flag"));
> + goto cleanup;
> + }
> + }
> +
> if ((pid = fork()) < 0) {
> ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> _("cannot fork child process: %s"), strerror(errno));
> @@ -132,33 +179,48 @@
> close(null);
> if (outfd) {
> close(pipeout[1]);
> - if(non_block)
> - if(virSetNonBlock(pipeout[0]) == -1)
> - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> - _("Failed to set non-blocking file descriptor flag"));
> -
> - if(virSetCloseExec(pipeout[0]) == -1)
> - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> - _("Failed to set close-on-exec file descriptor flag"));
> *outfd = pipeout[0];
> }
> if (errfd) {
> close(pipeerr[1]);
> - if(non_block)
> - if(virSetNonBlock(pipeerr[0]) == -1)
> - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> - _("Failed to set non-blocking file descriptor flag"));
> -
> - if(virSetCloseExec(pipeerr[0]) == -1)
> - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> - _("Failed to set close-on-exec file descriptor flag"));
> *errfd = pipeerr[0];
> }
> *retpid = pid;
> +
> + /* Restore our original signal mask now child is safely
> + running */
> + if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) < 0) {
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("cannot unblock signals: %s"),
> + strerror(errno));
> + return -1;
> + }
> +
> return 0;
> }
>
> /* child */
> +
> + /* Clear out all signal handlers from parent so nothing
> + unexpected can happen in our child here */
> + sig_action.sa_handler = SIG_DFL;
> + sig_action.sa_flags = 0;
> + sigemptyset(&sig_action.sa_mask);
> +
> + for (i = 0 ; i < NSIG ; i++)
I'd start at 1, since afaik, sigaction(0, ... serves no purpose.
> + /* Only possible errors are EFAULT or EINVAL
> + The former wont happen, the latter we
> + expect, so no need to check return value */
> + sigaction(i, &sig_action, NULL);
> +
> + /* Unmask all signals in child, since we've no idea
> + what the caller's done with their signal mask */
> + if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) < 0) {
> + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("cannot unblock signals: %s"),
> + strerror(errno));
> + return -1;
> + }
>
> if (pipeout[0] > 0 && close(pipeout[0]) < 0)
> _exit(1);
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list