[Spice-devel] [spice-gtk v1] gstaudio: fix ci due potential null pointer dereference

[Victor Toso]

Hi,

On Tue, Jan 22, 2019 at 08:01:33AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > Can't happen as GstElement *e is not NULL but doing extra G_OBJECT(e) cast
> > guarantees this for compiler.
> > 
> >  > spice-gstaudio.c: In function ‘playback_volume_changed’:
> >  > /usr/include/glib-2.0/gobject/gtype.h:2280:70: error: potential null
> >  > pointer dereference [-Werror=null-dereference]
> >  >  #define _G_TYPE_IGC(ip, gt, ct)         ((ct*) (((GTypeInstance*)
> >  >  ip)->g_class))
> >  >                                                 ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
> >  > /usr/include/glib-2.0/gobject/gtype.h:525:66: note: in expansion of macro
> >  > ‘_G_TYPE_IGC’
> >  >  #define G_TYPE_INSTANCE_GET_CLASS(instance, g_type, c_type)
> >  >  (_G_TYPE_IGC ((instance), (g_type), c_type))
> >  >                                                                   ^~~~~~~~~~~
> >  > /usr/include/glib-2.0/gobject/gobject.h:86:38: note: in expansion of macro
> >  > ‘G_TYPE_INSTANCE_GET_CLASS’
> >  >  #define G_OBJECT_GET_CLASS(object)  (G_TYPE_INSTANCE_GET_CLASS ((object),
> >  >  G_TYPE_OBJECT, GObjectClass))
> >  >                                       ^~~~~~~~~~~~~~~~~~~~~~~~~
> >  > spice-gstaudio.c:373:45: note: in expansion of macro ‘G_OBJECT_GET_CLASS’
> >  >      } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> >  >      "volume") != NULL) {
> >                                                  ^~~~~~~~~~~~~~~~~~
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> 
> Why not adding g_return_if_fail(e != NULL) before using "e" instead ?

Why that would be better than a proper G_OBJECT()?

We already check if playback.sink or record.src are null early on
in those functions, adding g_return_if_fail() would only check if
the assignment was done. I don't think it is needed.

Cheers,

> > ---
> > 
> >  Working CI:
> >  https://gitlab.freedesktop.org/victortoso/spice-gtk/pipelines/15885
> > 
> >  src/spice-gstaudio.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > index 51ff028..b2d9cd3 100644
> > --- a/src/spice-gstaudio.c
> > +++ b/src/spice-gstaudio.c
> > @@ -370,7 +370,7 @@ static void playback_volume_changed(GObject *object,
> > GParamSpec *pspec, gpointer
> >  
> >      if (GST_IS_STREAM_VOLUME(e)) {
> >          gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> >          GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > -    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > "volume") != NULL) {
> > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > "volume") != NULL) {
> >          g_object_set(e, "volume", vol, NULL);
> >      } else {
> >          g_warning("playback: ignoring volume change on %s",
> >          gst_element_get_name(e));
> > @@ -399,7 +399,7 @@ static void playback_mute_changed(GObject *object,
> > GParamSpec *pspec, gpointer d
> >  
> >      if (GST_IS_STREAM_VOLUME(e)) {
> >          gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > -    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute")
> > != NULL) {
> > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > "mute") != NULL) {
> >          g_object_set(e, "mute", mute, NULL);
> >      } else {
> >          g_warning("playback: ignoring mute change on %s",
> >          gst_element_get_name(e));
> > @@ -437,7 +437,7 @@ static void record_volume_changed(GObject *object,
> > GParamSpec *pspec, gpointer d
> >  
> >      if (GST_IS_STREAM_VOLUME(e)) {
> >          gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> >          GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > -    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > "volume") != NULL) {
> > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > "volume") != NULL) {
> >          g_object_set(e, "volume", vol, NULL);
> >      } else {
> >          g_warning("record: ignoring volume change on %s",
> >          gst_element_get_name(e));
> > @@ -466,7 +466,7 @@ static void record_mute_changed(GObject *object,
> > GParamSpec *pspec, gpointer dat
> >  
> >      if (GST_IS_STREAM_VOLUME (e)) {
> >          gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > -    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute")
> > != NULL) {
> > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > "mute") != NULL) {
> >          g_object_set(e, "mute", mute, NULL);
> >      } else {
> >          g_warning("record: ignoring mute change on %s",
> >          gst_element_get_name(e));
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190122/e39a28b7/attachment.sig>