thread unsafety in gst_pad_get_internal_links()

[Olivier Crête-2]

Hello,

I just noticed that gst_pad_get_internal_links() is not thread safe. It
does not take the object lock while looking up the list of pads and does
not reference the pads. So its very possible that it could happen in the
middle of a modification of the list (lets say if I add a pad to a tee).
Or that the pad could be gone by the time I get to do something with it.

I propose deprecating it (since we can't modify it without breaking the
ABI) and add a new call get_internal_links_safe() that will returns a
list of refed pads. And fixing the default implementation of boths to be
thread safe (and probably having the default implementation of the
unsafe version call the safe version and unref the pads).

--
Olivier Crête
[hidden email]
Collabora Ltd

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Eric Zhang-6]

Hi, gstreamer-devel:

    `gst_pad_get_internal_links' just call the `intlinkfunc' function
pointer which is set by user by calling
`gst_pad_set_internal_link_function'. And the lock is suppose to be set
on user's `internal_link' function, so I think there is no need to set
lock in `gst_pad_get_internal_links'.

    Here is the source code of `gst_pad_get_internal_links':

=================================
GList *
gst_pad_get_internal_links (GstPad * pad)
{
  GList *res = NULL;

  g_return_val_if_fail (GST_IS_PAD (pad), NULL);

  if (GST_PAD_INTLINKFUNC (pad))
    res = GST_PAD_INTLINKFUNC (pad) (pad);

  return res;
}

GST_PAD_INTLINKFUNC is defined as:

#define GST_PAD_INTLINKFUNC(pad)    (GST_PAD_CAST(pad)->intlinkfunc)
=================================

Eric Zhang


Olivier Crête wrote:

> Hello,
>
> I just noticed that gst_pad_get_internal_links() is not thread safe. It
> does not take the object lock while looking up the list of pads and does
> not reference the pads. So its very possible that it could happen in the
> middle of a modification of the list (lets say if I add a pad to a tee).
> Or that the pad could be gone by the time I get to do something with it.
>
> I propose deprecating it (since we can't modify it without breaking the
> ABI) and add a new call get_internal_links_safe() that will returns a
> list of refed pads. And fixing the default implementation of boths to be
> thread safe (and probably having the default implementation of the
> unsafe version call the safe version and unref the pads).
>
>  
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> Build the coolest Linux based applications with Moblin SDK & win great prizes
> Grand prize is a trip for two to an Open Source event anywhere in the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> ------------------------------------------------------------------------
>
> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel
>  

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Olivier Crête-2]

On Tue, 2008-08-26 at 10:46 +0800, Eric Zhang wrote:
> Hi, gstreamer-devel:
>
>     `gst_pad_get_internal_links' just call the `intlinkfunc' function
> pointer which is set by user by calling
> `gst_pad_set_internal_link_function'. And the lock is suppose to be set
> on user's `internal_link' function, so I think there is no need to set
> lock in `gst_pad_get_internal_links'.

I'm not talking about the gst_pad_get_internal_links(), but about its
default implementation.. (and about the fact that the returned pads dont
have their refcount increased).

Olivier

> Olivier Crête wrote:
> > Hello,
> >
> > I just noticed that gst_pad_get_internal_links() is not thread safe. It
> > does not take the object lock while looking up the list of pads and does
> > not reference the pads. So its very possible that it could happen in the
> > middle of a modification of the list (lets say if I add a pad to a tee).
> > Or that the pad could be gone by the time I get to do something with it.
> >
> > I propose deprecating it (since we can't modify it without breaking the
> > ABI) and add a new call get_internal_links_safe() that will returns a
> > list of refed pads. And fixing the default implementation of boths to be
> > thread safe (and probably having the default implementation of the
> > unsafe version call the safe version and unref the pads).
> >
> >  
> > ------------------------------------------------------------------------
> >
> > -------------------------------------------------------------------------
> > This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> > Build the coolest Linux based applications with Moblin SDK & win great prizes
> > Grand prize is a trip for two to an Open Source event anywhere in the world
> > http://moblin-contest.org/redirect.php?banner_id=100&url=/
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > gstreamer-devel mailing list
> > [hidden email]
> > https://lists.sourceforge.net/lists/listinfo/gstreamer-devel
> >  
>
>
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> Build the coolest Linux based applications with Moblin SDK & win great prizes
> Grand prize is a trip for two to an Open Source event anywhere in the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

--

Olivier Crête
[hidden email]
Collabora Ltd

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel