[Spice-devel] [PATCH spice-xpi 1/4] Avoid a race leading to a plugin crash

[Alon Levy]

On Fri, Feb 24, 2012 at 11:54:35AM +0100, Marc-André Lureau wrote:
> When the child exits quickly (failed to start etc..), it doesn't get
> tracked properly and when receiving sigchld, "fake_this" may be NULL
> leading to a crash (fake_this = s_children[info->si_pid])
> ---
>  SpiceXPI/src/plugin/plugin.cpp |   34 +++++++++++++++++++++++++++++++---
>  1 files changed, 31 insertions(+), 3 deletions(-)
> 

ACK. Just minor nitpick and silly question.

> diff --git a/SpiceXPI/src/plugin/plugin.cpp b/SpiceXPI/src/plugin/plugin.cpp
> index b82d074..c3d2d91 100644
> --- a/SpiceXPI/src/plugin/plugin.cpp
> +++ b/SpiceXPI/src/plugin/plugin.cpp
> @@ -569,10 +569,27 @@ void nsPluginInstance::Connect()
>          return;
>      }
>  
> +    /* use a pipe for the children to wait until it gets tracked */
> +    int pipe_fds[2] = { -1, -1 };
empty line here

> +    if (pipe(pipe_fds) < 0) {
> +        perror("spice-xpi system error");
> +        return;
> +    }
> +
>      pid_t child = fork();
>      LOG_DEBUG("child pid: " << child);
>      if (child == 0)
>      {
> +        close (pipe_fds[1]);
> +        pipe_fds[1] = -1;
> +
> +        char c;
> +        if (read(pipe_fds[0], &c, 1) != 0)
> +            LOG_ERROR("Invalid read on pipe");
> +
> +        close (pipe_fds[0]);
> +        pipe_fds[0] = -1;
> +
>          execl("/usr/libexec/spice-xpi-client", "/usr/libexec/spice-xpi-client", NULL);
>          LOG_ERROR("ERROR failed to run spice-xpi-client");
>  
> @@ -584,6 +601,14 @@ void nsPluginInstance::Connect()
>      }
>      else
>      {
> +        close(pipe_fds[0]);
> +        pipe_fds[0] = -1;
> +
> +        s_children[child] = this;
> +
> +        close(pipe_fds[1]);
> +        pipe_fds[1] = -1;
> +

So you're not doing a write, but instead just a close, and so the
child's read should return 0?

>          m_external_controller.SetFilename(socket_file);
>  
>          if (m_external_controller.Connect(10) != 0)
> @@ -659,8 +684,6 @@ void nsPluginInstance::Connect()
>  
>          // set connected status
>          m_connected_status = -1;
> -
> -        s_children[child] = this;
>      }
>  }
>  
> @@ -765,6 +788,11 @@ void nsPluginInstance::SigchldRoutine(int sig, siginfo_t *info, void *uap)
>  
>      if (!getenv("SPICE_XPI_DEBUG")) {
>          nsPluginInstance *fake_this = s_children[info->si_pid];
> +        if (fake_this == NULL) {
> +            LOG_ERROR("Invalid children signal");
> +            return;
> +        }
> +
>          fake_this->CallOnDisconnected(exit_code);
>          fake_this->m_external_controller.Disconnect();
>      }
> @@ -777,7 +805,7 @@ void nsPluginInstance::SigchldRoutine(int sig, siginfo_t *info, void *uap)
>  // ==============================
>  //
>  // here the plugin is asked by Mozilla to tell if it is scriptable
> -// we should return a valid interface id and a pointer to 
> +// we should return a valid interface id and a pointer to
>  // nsScriptablePeer interface which we should have implemented
>  // and which should be defined in the corressponding *.xpt file
>  // in the bin/components folder
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel