Bug 79006

Created attachment 99477 [details] [review]
contact-list: don't crash if 'd' is NULL

Created attachment 99478 [details] [review]
tp-fs: fix a potential NULL dereferencing crash

Created attachment 99479 [details] [review]
tp-fs: add mising 'break' in switch block

Created attachment 99480 [details] [review]
account-request: remove useless 'priv' variable

Created attachment 99481 [details] [review]
ensure that values received from callback are not NULL

Created attachment 99482 [details] [review]
ensure that class pointers aren't NULL

Created attachment 99483 [details] [review]
base-client: fix potential uninitialized variable bug

Created attachment 99484 [details] [review]
logger: don't assume @error is not NULL

Created attachment 99485 [details] [review]
test-connection-error: don't check errors twice

Comment on attachment 99483 [details] [review]
base-client: fix potential uninitialized variable bug

>From 6c5f392d9487a94a4fa62c1aaaebdc18794dea9d Mon Sep 17 00:00:00 2001
>From: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
>Date: Wed, 21 May 2014 10:52:49 +0200
>Subject: [PATCH 7/9] base-client: fix potential uninitialized variable bug
>
>https://bugs.freedesktop.org/show_bug.cgi?id=79006
>---
> telepathy-glib/base-client.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/telepathy-glib/base-client.c b/telepathy-glib/base-client.c
>index cb717ca..e9040e2 100644
>--- a/telepathy-glib/base-client.c
>+++ b/telepathy-glib/base-client.c
>@@ -2328,7 +2328,7 @@ _tp_base_client_add_request (TpSvcClientInterfaceRequests *iface,
> {
>   TpBaseClient *self = TP_BASE_CLIENT (iface);
>   TpChannelRequest *request;
>-  TpAccount *account;
>+  TpAccount *account = NULL;
>   GError *error = NULL;
>   channel_request_prepare_account_ctx *ctx;
>   GArray *account_features;
>-- 
>1.9.0
>

(In reply to comment #10)
> Comment on attachment 99483 [details] [review] [review]
> base-client: fix potential uninitialized variable bug
> 
> >From 6c5f392d9487a94a4fa62c1aaaebdc18794dea9d Mon Sep 17 00:00:00 2001
> >From: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
> >Date: Wed, 21 May 2014 10:52:49 +0200
> >Subject: [PATCH 7/9] base-client: fix potential uninitialized variable bug
> >
> >https://bugs.freedesktop.org/show_bug.cgi?id=79006
> >---
> > telepathy-glib/base-client.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/telepathy-glib/base-client.c b/telepathy-glib/base-client.c
> >index cb717ca..e9040e2 100644
> >--- a/telepathy-glib/base-client.c
> >+++ b/telepathy-glib/base-client.c
> >@@ -2328,7 +2328,7 @@ _tp_base_client_add_request (TpSvcClientInterfaceRequests *iface,
> > {
> >   TpBaseClient *self = TP_BASE_CLIENT (iface);
> >   TpChannelRequest *request;
> >-  TpAccount *account;
> >+  TpAccount *account = NULL;
> >   GError *error = NULL;
> >   channel_request_prepare_account_ctx *ctx;
> >   GArray *account_features;
> >-- 
> >1.9.0
> >

Ooops, sorry; ignore that quote.

Comment on attachment 99481 [details] [review]
ensure that values received from callback are not NULL

Review of attachment 99481 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/account.c
@@ +3313,4 @@
>  {
>    GTask *task = user_data;
>  
> +  g_return_if_fail (out_Value);

This shouldn't be asserted until after @error has been checked?

::: telepathy-glib/base-room-config.c
@@ +759,4 @@
>    static TpDBusPropertiesMixinIfaceInfo *iface_info = NULL;
>    TpDBusPropertiesMixinPropInfo *prop_info;
>  
> +  g_return_val_if_fail (value != NULL, FALSE);

(unreviewed, I'll need to see more context)

::: telepathy-glib/connection.c
@@ +515,4 @@
>    TpConnection *self = (TpConnection *) proxy;
>    GSimpleAsyncResult *result;
>  
> +  g_return_if_fail (value != NULL);

Should not be asserted until @error has been checked

::: telepathy-glib/debug-client.c
@@ +268,4 @@
>  {
>    TpDebugClient *self = TP_DEBUG_CLIENT (proxy);
>  
> +  g_return_if_fail (value != NULL);

Likewise

Comment on attachment 99479 [details] [review]
tp-fs: add mising 'break' in switch block

Review of attachment 99479 [details] [review]:
-----------------------------------------------------------------

This one seems worth fixing in stable too

Comment on attachment 99478 [details] [review]
tp-fs: fix a potential NULL dereferencing crash

Review of attachment 99478 [details] [review]:
-----------------------------------------------------------------

::: telepathy-farstream/call-stream.c
@@ +331,5 @@
>        tf_call_stream_fail (self,
>            TP_CALL_STATE_CHANGE_REASON_INTERNAL_ERROR,
>            TP_ERROR_STR_MEDIA_STREAMING_ERROR,
> +          "Error setting the remote candidates: %s",
> +          error != NULL ? error->message : "invalid transport type");

This usually indicates that GError is being used incorrectly; I'll have to look at more context.

Comment on attachment 99483 [details] [review]
base-client: fix potential uninitialized variable bug

Review of attachment 99483 [details] [review]:
-----------------------------------------------------------------

++, worth fixing in stable

Comment on attachment 99484 [details] [review]
logger: don't assume @error is not NULL

Review of attachment 99484 [details] [review]:
-----------------------------------------------------------------

::: telepathy-logger/log-store-xml.c
@@ +451,5 @@
>    bus_connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, error);
>    if (bus_connection == NULL)
>      {
> +      DEBUG ("Error acquiring bus connection: %s",
> +          error != NULL ? (*error)->message : "");

I'd prefer "(no GError available)" over "" as the fallback.

All OK except where noted. Please apply to stable and master where noted. For changes not suitable for stable, please apply to master if it's easy to do so.

Created attachment 99859 [details] [review]
ensure that values received from callback are not NULL

fixed.

(In reply to comment #13)
> Comment on attachment 99479 [details] [review] [review]
> tp-fs: add mising 'break' in switch block
> 
> Review of attachment 99479 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This one seems worth fixing in stable too

Fixed in tp-fs master as well (we didn't branch for 0.6).

(In reply to comment #14)
> Comment on attachment 99478 [details] [review] [review]
> tp-fs: fix a potential NULL dereferencing crash
> 
> Review of attachment 99478 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-farstream/call-stream.c
> @@ +331,5 @@
> >        tf_call_stream_fail (self,
> >            TP_CALL_STATE_CHANGE_REASON_INTERNAL_ERROR,
> >            TP_ERROR_STR_MEDIA_STREAMING_ERROR,
> > +          "Error setting the remote candidates: %s",
> > +          error != NULL ? error->message : "invalid transport type");
> 
> This usually indicates that GError is being used incorrectly; I'll have to
> look at more context.

The only case where it can happen, assuming the called functions properly set the error if they failed, is the default switch case. We can just g_return_if_reached() if you prefer.

static void
_tf_call_stream_push_remote_candidates (TfCallStream *self,
    GList *fscandidates)
{
  gboolean ret;
  GError *error = NULL;

  switch (self->transport_type)
    {
    case TP_STREAM_TRANSPORT_TYPE_RAW_UDP:
    case TP_STREAM_TRANSPORT_TYPE_SHM:
    case TP_STREAM_TRANSPORT_TYPE_MULTICAST:
      ret = fs_stream_force_remote_candidates (self->fsstream,
          fscandidates, &error);
      break;
    case TP_STREAM_TRANSPORT_TYPE_ICE:
    case TP_STREAM_TRANSPORT_TYPE_GTALK_P2P:
    case TP_STREAM_TRANSPORT_TYPE_WLM_2009:
      ret = fs_stream_add_remote_candidates (self->fsstream, fscandidates,
          &error);
      break;
    default:
      ret = FALSE;
    }

  if (!ret)
    {
      tf_call_stream_fail (self,
          TP_CALL_STATE_CHANGE_REASON_INTERNAL_ERROR,
          TP_ERROR_STR_MEDIA_STREAMING_ERROR,
          "Error setting the remote candidates: %s",
          error != NULL ? error->message : "invalid transport type");
      g_clear_error (&error);
    }

Created attachment 99861 [details] [review]
ensure that values received from callback are not NULL

(In reply to comment #15)
> Comment on attachment 99483 [details] [review] [review]
> base-client: fix potential uninitialized variable bug
> 
> Review of attachment 99483 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ++, worth fixing in stable

Pushed to 0.24

All merged to master/next but:
- tp-fs: fix a potential NULL dereferencing crash
- ensure that values received from callback are not NULL

Comment on attachment 99478 [details] [review]
tp-fs: fix a potential NULL dereferencing crash

Review of attachment 99478 [details] [review]:
-----------------------------------------------------------------

With the additional context that you quoted: yes this is correct (if a bit weird), please merge.

Comment on attachment 99861 [details] [review]
ensure that values received from callback are not NULL

Review of attachment 99861 [details] [review]:
-----------------------------------------------------------------

This one looks fine now.

::: telepathy-glib/account.c
@@ +3312,5 @@
>      GObject *weak_object)
>  {
>    GTask *task = user_data;
>  
> +  g_return_if_fail (error != NULL || value != NULL);

Much better

::: telepathy-glib/base-room-config.c
@@ +759,4 @@
>    static TpDBusPropertiesMixinIfaceInfo *iface_info = NULL;
>    TpDBusPropertiesMixinPropInfo *prop_info;
>  
> +  g_return_val_if_fail (value != NULL, FALSE);

I've looked at the context now, and yes it's correct

All merged; thanks!