[libvirt] [PATCH glib] gobject-stream: fix issue found by coverity
The coverity server found issue in gvir_stream_close function that we ignore return values of g_input_stream_close and g_outpu_stream_close, but we also ignore the error message and we assume that it's closed without error. Now we will check return values and also propagate the error message to the upper layers. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- libvirt-gobject/libvirt-gobject-stream.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c index 1572022..66a12ab 100644 --- a/libvirt-gobject/libvirt-gobject-stream.c +++ b/libvirt-gobject/libvirt-gobject-stream.c @@ -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); + } if (self->priv->output_stream) - g_output_stream_close(self->priv->output_stream, cancellable, NULL); + o_ret = g_output_stream_close(self->priv->output_stream, cancellable, &local_error); + + if (local_error) { + if (!*error) + g_propagate_error(error, local_error); + else + g_error_free(local_error); + } - return TRUE; /* FIXME: really close the stream? */ + return (i_ret && o_ret); } -- 1.8.3.1
Hey, On Thu, Feb 20, 2014 at 01:49:31PM +0100, Pavel Hrdina wrote:
The coverity server found issue in gvir_stream_close function that we ignore return values of g_input_stream_close and g_outpu_stream_close, but we also ignore the error message and we
missing 't' in g_output_stream_close
assume that it's closed without error.
Now we will check return values and also propagate the error message to the upper layers.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- libvirt-gobject/libvirt-gobject-stream.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c index 1572022..66a12ab 100644 --- a/libvirt-gobject/libvirt-gobject-stream.c +++ b/libvirt-gobject/libvirt-gobject-stream.c @@ -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
On 20.2.2014 15:07, Christophe Fergeau wrote:
Hey,
On Thu, Feb 20, 2014 at 01:49:31PM +0100, Pavel Hrdina wrote:
The coverity server found issue in gvir_stream_close function that we ignore return values of g_input_stream_close and g_outpu_stream_close, but we also ignore the error message and we
missing 't' in g_output_stream_close
Oh, thanks, I'll fix that.
assume that it's closed without error.
Now we will check return values and also propagate the error message to the upper layers.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- libvirt-gobject/libvirt-gobject-stream.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-stream.c b/libvirt-gobject/libvirt-gobject-stream.c index 1572022..66a12ab 100644 --- a/libvirt-gobject/libvirt-gobject-stream.c +++ b/libvirt-gobject/libvirt-gobject-stream.c @@ -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. Pavel
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
participants (2)
-
Christophe Fergeau -
Pavel Hrdina