2011-05-27 11:26:24

by Dmitriy Paliy

Subject: [PATCH] Fix invalid read and possible memory leaks

Fixed incorrect update of transport->owners GSlist in media_transport_free.
Removal of list entries within 'for' loop leads to invalid read of memory
(l = l->next) and memory leaks.
---
audio/transport.c | 15 +++++++++++----
1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index aa3308d..4047d80 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -805,14 +805,21 @@ static GDBusSignalTable transport_signals[] = {
{ }
};

+static void media_transport_remove_helper(gpointer data, gpointer user_data)
+{
+ struct media_transport *transport = user_data;
+ struct media_owner *owner = data;
+
+ media_transport_remove(transport, owner);
+}
+
+
static void media_transport_free(void *data)
{
struct media_transport *transport = data;
- GSList *l;
-
- for (l = transport->owners; l; l = l->next)
- media_transport_remove(transport, l->data);

+ g_slist_foreach(transport->owners, media_transport_remove_helper,
+ transport);
g_slist_free(transport->owners);

if (transport->session)
--
1.7.4.1

2011-05-27 13:03:25

by Luiz Augusto von Dentz

Subject: Re: [PATCH] Fix invalid read and possible memory leaks

Hi Dmitriy,

On Fri, May 27, 2011 at 3:03 PM, Dmitriy Paliy <[email protected]> wrote:
> Hi Luiz,
>
> On Fri, 2011-05-27 at 14:56 +0300, ext Luiz Augusto von Dentz wrote:
>> Hi Dmitry,
>>
>> On Fri, May 27, 2011 at 2:26 PM, Dmitriy Paliy <[email protected]> wrote:
>> > Fixed incorrect update of transport->owners GSlist in media_transport_free.
>> > Removal of list entries within 'for' loop leads to invalid read of memory
>> > (l = l->next) and memory leaks.
>> > ---
>> > ?audio/transport.c | ? 15 +++++++++++----
>> > ?1 files changed, 11 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/audio/transport.c b/audio/transport.c
>> > index aa3308d..4047d80 100644
>> > --- a/audio/transport.c
>> > +++ b/audio/transport.c
>> > @@ -805,14 +805,21 @@ static GDBusSignalTable transport_signals[] = {
>> > ? ? ? ?{ }
>> > ?};
>> >
>> > +static void media_transport_remove_helper(gpointer data, gpointer user_data)
>> > +{
>> > + ? ? ? struct media_transport *transport = user_data;
>> > + ? ? ? struct media_owner *owner = data;
>> > +
>> > + ? ? ? media_transport_remove(transport, owner);
>> > +}
>> > +
>> > +
>> > ?static void media_transport_free(void *data)
>> > ?{
>> > ? ? ? ?struct media_transport *transport = data;
>> > - ? ? ? GSList *l;
>> > -
>> > - ? ? ? for (l = transport->owners; l; l = l->next)
>> > - ? ? ? ? ? ? ? media_transport_remove(transport, l->data);
>> >
>> > + ? ? ? g_slist_foreach(transport->owners, media_transport_remove_helper,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? transport);
>> > ? ? ? ?g_slist_free(transport->owners);
>> >
>> > ? ? ? ?if (transport->session)
>> > --
>> > 1.7.4.1
>>
>> Usually we should just use while (list) and do the list = list->next
>> before item removal.
>
> This is how it is done in the g_slist_foreach function. Therefore, may
> be it is better to use ready function without exposing implementation
> details of this loop.

Well normally it is more code to do it via g_slist_foreach than on
while loop, it is probably more expensive too since it has to call a
callback for each element and obviously the g_slist_foreach has to
take care of prefetch next element.

--
Luiz Augusto von Dentz
Computer Engineer

2011-05-27 12:03:23

by Dmitriy Paliy

Subject: Re: [PATCH] Fix invalid read and possible memory leaks

Hi Luiz,

On Fri, 2011-05-27 at 14:56 +0300, ext Luiz Augusto von Dentz wrote:
> Hi Dmitry,
>
> On Fri, May 27, 2011 at 2:26 PM, Dmitriy Paliy <[email protected]> wrote:
> > Fixed incorrect update of transport->owners GSlist in media_transport_free.
> > Removal of list entries within 'for' loop leads to invalid read of memory
> > (l = l->next) and memory leaks.
> > ---
> > audio/transport.c | 15 +++++++++++----
> > 1 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/audio/transport.c b/audio/transport.c
> > index aa3308d..4047d80 100644
> > --- a/audio/transport.c
> > +++ b/audio/transport.c
> > @@ -805,14 +805,21 @@ static GDBusSignalTable transport_signals[] = {
> > { }
> > };
> >
> > +static void media_transport_remove_helper(gpointer data, gpointer user_data)
> > +{
> > + struct media_transport *transport = user_data;
> > + struct media_owner *owner = data;
> > +
> > + media_transport_remove(transport, owner);
> > +}
> > +
> > +
> > static void media_transport_free(void *data)
> > {
> > struct media_transport *transport = data;
> > - GSList *l;
> > -
> > - for (l = transport->owners; l; l = l->next)
> > - media_transport_remove(transport, l->data);
> >
> > + g_slist_foreach(transport->owners, media_transport_remove_helper,
> > + transport);
> > g_slist_free(transport->owners);
> >
> > if (transport->session)
> > --
> > 1.7.4.1
>
> Usually we should just use while (list) and do the list = list->next
> before item removal.

This is how it is done in the g_slist_foreach function. Therefore, may
be it is better to use ready function without exposing implementation
details of this loop.

BR,
Dmitriy

2011-05-27 11:56:18

by Luiz Augusto von Dentz

Subject: Re: [PATCH] Fix invalid read and possible memory leaks

Hi Dmitry,

On Fri, May 27, 2011 at 2:26 PM, Dmitriy Paliy <[email protected]> wrote:
> Fixed incorrect update of transport->owners GSlist in media_transport_free.
> Removal of list entries within 'for' loop leads to invalid read of memory
> (l = l->next) and memory leaks.
> ---
> ?audio/transport.c | ? 15 +++++++++++----
> ?1 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index aa3308d..4047d80 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -805,14 +805,21 @@ static GDBusSignalTable transport_signals[] = {
> ? ? ? ?{ }
> ?};
>
> +static void media_transport_remove_helper(gpointer data, gpointer user_data)
> +{
> + ? ? ? struct media_transport *transport = user_data;
> + ? ? ? struct media_owner *owner = data;
> +
> + ? ? ? media_transport_remove(transport, owner);
> +}
> +
> +
> ?static void media_transport_free(void *data)
> ?{
> ? ? ? ?struct media_transport *transport = data;
> - ? ? ? GSList *l;
> -
> - ? ? ? for (l = transport->owners; l; l = l->next)
> - ? ? ? ? ? ? ? media_transport_remove(transport, l->data);
>
> + ? ? ? g_slist_foreach(transport->owners, media_transport_remove_helper,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? transport);
> ? ? ? ?g_slist_free(transport->owners);
>
> ? ? ? ?if (transport->session)
> --
> 1.7.4.1

Usually we should just use while (list) and do the list = list->next
before item removal.

--
Luiz Augusto von Dentz
Computer Engineer