[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH glib] gobject-stream: fix issue found by coverity



On Thu, Feb 20, 2014 at 03:57:00PM +0100, Pavel Hrdina wrote:
> On 20.2.2014 15:07, Christophe Fergeau wrote:
> >>@@ -102,17 +102,33 @@ static GOutputStream* gvir_stream_get_output_stream(GIOStream *io_stream)
> >>
> >>  static gboolean gvir_stream_close(GIOStream *io_stream,
> >>                                    GCancellable *cancellable,
> >>-                                  G_GNUC_UNUSED GError **error)
> >>+                                  GError **error)
> >>  {
> >>      GVirStream *self = GVIR_STREAM(io_stream);
> >>+    GError *local_error = NULL;
> >>+    gboolean i_ret = TRUE, o_ret = TRUE;
> >>
> >>      if (self->priv->input_stream)
> >>-        g_input_stream_close(self->priv->input_stream, cancellable, NULL);
> >>+        i_ret = g_input_stream_close(self->priv->input_stream, cancellable, &local_error);
> >>+
> >>+    if (local_error) {
> >>+        if (!*error)
> >>+            g_propagate_error(error, local_error);
> >>+        else
> >>+            g_error_free(local_error);
> >>+    }
> >
> >g_propagate_error will be doing this if (!*error) check for you, so this
> >can be written as
> >if (local_error)
> >     g_propagate_error(error, local_error);
> >
> >
> >
> >void                g_propagate_error                   (GError **dest,
> >                                                          GError *src);
> >If dest is NULL, free src; otherwise, moves src into *dest.
> >
> >
> >Christophe
> >
> 
> The g_propagate_error will free the src only if error == NULL but if
> the *error is not NULL it will print warning and do nothing with the
> src and therefore there could be memory leak. That's why I'm checking
> if *error is null and otherwise I'll free the local_error.

If error is NULL, this test will cause a NULL pointer dereference.
Passing a GError ** pointing to a non-NULL GError is a programming error,
so what is done in other places in libvirt-glib is to have a
g_return_val_if_fail(error == NULL || *error == NULL, FALSE);
at the beginning of the function to reject such invalid calls right away at
runtime.
If you do that, the first check and g_error_free() can be removed.
If an error occurred there, and if the g_output_stream_close() call fails,
we'll be in a situation when g_propagate_error() can try to overwrite an
already set error. g_io_stream_close() says "On failure the first error
that happened will be reported, but the close operation will finish as much
as possible." so we need to do the g_output_stream_close() call even if the
g_input_stream_close() call failed, but not try to set the error when this
happened.

Christophe

Attachment: pgpXD75wlLt6n.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]