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

[Peter Maydell]

On 15 May 2012 15:22, Michael Roth <address@hidden> wrote:
> On Tue, May 15, 2012 at 02:32:41PM +0100, Peter Maydell wrote:
>> On 14 May 2012 23:04, 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;
>> >         }
>> >     }
>>
>> 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.

I think there's a dislike of middle-of-block declarations (C++/C99 style),
but beginning of a block is certainly fine.

-- PMM