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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Tue, May 15, 2012 at 02:32:41PM +0100, Peter Maydell wrote:
> On 14 May 2012 23:04, 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;
> >         }
> >     }
> 
> It would be nicer to put the log_file variable definition
> inside the if(), rather than putting it 150 lines earlier
> in the source file...

Agreed...I've had it in my head for the longest time that declarations
at the beginning of functions were QEMU coding style, but looking again
I'm not sure where I got that idea.

Fixed in tree:

https://github.com/mdroth/qemu/commit/6c615ec57e83bf8cc7b1721bcd58c7d1ed93ef65

> 
> -- PMM
>