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

[Michael Roth]

On Tue, May 15, 2012 at 10:04:32AM -0300, Luiz Capitulino wrote:
> On Mon, 14 May 2012 17:04:17 -0500
> Michael Roth <address@hidden> 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 <address@hidden>
> > ---
> >  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? g_log_set_default_handler() does
do locking, but our log handler is actually unchanged, so the only case
it would be needed is if the opaque argument changes, or there were
multiple fields changed in the opaque, since that might result it an
undefined mix of old/new values.

The locking for the actual fields in the opaque is our responsibility,
and there's no way to get glib to comply (short of duplicating the
opaque and then using g_log_set_default_handler(), since g_messages_lock
isn't exposed)

But since we're only changing a single field, the FILE* the log handler
passes to fprintf(), we get the same level of automicity: a log
statement will get stuck either in stderr or the new handle, depending
on whether we made this change before or after another caller got to
that point in ga_log(). So we should be okay, and aren't violating any
glib locking protocols with change.

> 
> >          }
> >      }
> >  
> 
>