| Message ID | 1334777449-18542-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 18, 2012 at 04:30:48PM -0300, 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 <lcapitulino@redhat.com> Looks good. > --- > 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; > -- > 1.7.9.2.384.g4a92a > >
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 <lcapitulino@redhat.com> > --- > 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
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;
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 <lcapitulino@redhat.com> --- qga/commands-posix.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)