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

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

[Michael Roth]

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;
         }
     }
 
-- 
1.7.4.1

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

[Michal Privoznik]

On 15.05.2012 00: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 <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;
>          }
>      }
>  

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

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

[Luiz Capitulino]

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?

>          }
>      }
>  

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

[Peter Maydell]

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...

-- PMM

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 <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? 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.

> 
> >          }
> >      }
> >  
> 
> 

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

[Michael Roth]

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
> 

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 <mdroth@linux.vnet.ibm.com> wrote:
> 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.

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

-- PMM

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

[Luiz Capitulino]

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>