I wanted to give Tartan a shot to see if he could help us finding potential regressions (like signals removing/renaming) in next. I did not manage to use it but find a bunch of bugs and potential issues by using clang alone. Some of those patches are less useful than others but couldn't hurt from a safer programming pov.
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!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.