Re: [Qemu-devel] [PATCH 1.1] qemu-ga: fix segv after failure to open log file

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 15 May 2012 09:14:13 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Tue, May 15, 2012 at 10:04:32AM -0300, Luiz Capitulino wrote:
> > On Mon, 14 May 2012 17:04:17 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > Currently, if we fail to open the specified log file (generally due to a
> > > permissions issue), we'll assign NULL to the logfile handle (stderr,
> > > initially) used by the logging routines, which can cause a segfault to
> > > occur when we attempt to report the error before exiting.
> > > 
> > > Instead, only re-assign if the open() was successful.
> > > 
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > ---
> > >  qemu-ga.c |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qemu-ga.c b/qemu-ga.c
> > > index 3a88333..e2725c8 100644
> > > --- a/qemu-ga.c
> > > +++ b/qemu-ga.c
> > > @@ -681,6 +681,7 @@ int main(int argc, char **argv)
> > >      const char *log_filepath = NULL;
> > >      const char *pid_filepath = QGA_PIDFILE_DEFAULT;
> > >      const char *state_dir = QGA_STATEDIR_DEFAULT;
> > > +    FILE *log_file;
> > >  #ifdef _WIN32
> > >      const char *service = NULL;
> > >  #endif
> > > @@ -836,12 +837,13 @@ int main(int argc, char **argv)
> > >              become_daemon(pid_filepath);
> > >          }
> > >          if (log_filepath) {
> > > -            s->log_file = fopen(log_filepath, "a");
> > > -            if (!s->log_file) {
> > > +            log_file = fopen(log_filepath, "a");
> > > +            if (!log_file) {
> > >                  g_critical("unable to open specified log file: %s",
> > >                             strerror(errno));
> > >                  goto out_bad;
> > >              }
> > > +            s->log_file = log_file;
> > 
> > Is it safe to change the log file this way? Isn't it necessary
> > to go through g_log_set_default_handler() or some other function?
> 
> Are you worried about a race condition?

Actually, I was worried that glib could store the old s->log_file pointer
somewhere, but I think I misread its documentation. Now I see that it's only
used by ga_log().

I think that doing the log setup before logging anything would be the best
solution, but that's a future improvement:

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>