Re: [Qemu-devel] [PATCH 1/2] qemu-ga: suspend: fix possible SIGCHLD during close() and g_free()

[Laszlo Ersek]

On 04/18/12 21:30, Luiz Capitulino wrote:
> A child created by bios_supports_mode() could terminate during the call
> to close() or g_free(). This could cause the SIGCHLD signal to be
> deliveried in the midle of their execution. Possible problems range from
> resource leak to segfault. Fix that by blocking SIGCHLD during those calls.
> 
> Also, tries to explain why bios_supports_mode() got so complex...
> 
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  qga/commands-posix.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index faf970d..41ba0c5 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -521,14 +521,18 @@ static void guest_fsfreeze_cleanup(void)
>   * This function forks twice and the information about the mode support
>   * status is passed to the qemu-ga process via a pipe.
>   *
> - * This approach allows us to keep the way we reap terminated children
> - * in qemu-ga quite simple.
> + * XXX: This approach is a bit complex, it's implemented this way to avoid
> + *      calling waitpid() from the main qemu-ga process, as this could cause
> + *      interference with other commands that create new processes. The
> + *      solution to this problem is to introduce an internal API to safely
> + *      create & wait for children processes.
>   */
>  static void bios_supports_mode(const char *pmutils_bin, const char 
> *pmutils_arg,
>                                 const char *sysfile_str, Error **err)
>  {
>      pid_t pid;
>      ssize_t ret;
> +    sigset_t sigset;
>      char *pmutils_path;
>      int status, pipefds[2];
>  
> @@ -603,9 +607,15 @@ static void bios_supports_mode(const char *pmutils_bin, 
> const char *pmutils_arg,
>          _exit(EXIT_SUCCESS);
>      }
>  
> +    sigemptyset(&sigset);
> +    sigaddset(&sigset, SIGCHLD);
> +    pthread_sigmask(SIG_BLOCK, &sigset, NULL);
> +
>      close(pipefds[1]);
>      g_free(pmutils_path);
>  
> +    pthread_sigmask(SIG_UNBLOCK, &sigset, NULL);
> +
>      if (pid < 0) {
>          error_set(err, QERR_UNDEFINED_ERROR);
>          goto out;

If "pid < 0" then we shouldn't have to expect a SIGCHLD, indeed. (I was
a bit worried about error_set().)

Looks good to me too.

Laszlo