[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: Block & reset signals when fork/exec'ing children



"Daniel P. Berrange" <berrange redhat com> 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);


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]