From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43410) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUIeR-0000bq-2F for qemu-devel@nongnu.org; Tue, 15 May 2012 10:23:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUIeH-00045H-1n for qemu-devel@nongnu.org; Tue, 15 May 2012 10:22:58 -0400 Received: from mail-gg0-f173.google.com ([209.85.161.173]:49310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUIeG-00043V-Oa for qemu-devel@nongnu.org; Tue, 15 May 2012 10:22:48 -0400 Received: by ggnp1 with SMTP id p1so5499459ggn.4 for ; Tue, 15 May 2012 07:22:47 -0700 (PDT) Sender: fluxion Date: Tue, 15 May 2012 09:22:42 -0500 From: Michael Roth Message-ID: <20120515142242.GB2967@illuin> References: <1337033057-31707-1-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1.1] qemu-ga: fix segv after failure to open log file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, lcapitulino@redhat.com On Tue, May 15, 2012 at 02:32:41PM +0100, Peter Maydell wrote: > On 14 May 2012 23:04, Michael Roth 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 > > --- > >  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 >