* [PATCH 1/1] media: Set entity->links NULL in cleanup
@ 2014-05-27 13:27 Sakari Ailus
2014-07-17 11:43 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2014-05-27 13:27 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart
Calling media_entity_cleanup() on a cleaned-up entity would result into
double free of the entity->links pointer and likely memory corruption as
well. Setting entity->links as NULL right after the kfree() avoids this.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-entity.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 37c334e..c404354 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -83,6 +83,7 @@ void
media_entity_cleanup(struct media_entity *entity)
{
kfree(entity->links);
+ entity->links = NULL;
}
EXPORT_SYMBOL_GPL(media_entity_cleanup);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: Set entity->links NULL in cleanup
2014-05-27 13:27 [PATCH 1/1] media: Set entity->links NULL in cleanup Sakari Ailus
@ 2014-07-17 11:43 ` Laurent Pinchart
2014-07-17 11:53 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2014-07-17 11:43 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thank you for the patch.
On Tuesday 27 May 2014 16:27:49 Sakari Ailus wrote:
> Calling media_entity_cleanup() on a cleaned-up entity would result into
> double free of the entity->links pointer and likely memory corruption as
> well.
My first question is, why would anyone do that ? :-)
> Setting entity->links as NULL right after the kfree() avoids this.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/media-entity.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 37c334e..c404354 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -83,6 +83,7 @@ void
> media_entity_cleanup(struct media_entity *entity)
> {
> kfree(entity->links);
> + entity->links = NULL;
> }
> EXPORT_SYMBOL_GPL(media_entity_cleanup);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: Set entity->links NULL in cleanup
2014-07-17 11:43 ` Laurent Pinchart
@ 2014-07-17 11:53 ` Sakari Ailus
2014-07-17 12:13 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2014-07-17 11:53 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media
Hi Laurent,
On Thu, Jul 17, 2014 at 01:43:09PM +0200, Laurent Pinchart wrote:
> On Tuesday 27 May 2014 16:27:49 Sakari Ailus wrote:
> > Calling media_entity_cleanup() on a cleaned-up entity would result into
> > double free of the entity->links pointer and likely memory corruption as
> > well.
>
> My first question is, why would anyone do that ? :-)
Because it makes error handling easier. Many cleanup functions work this
way, but not media_entity_cleanup().
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: Set entity->links NULL in cleanup
2014-07-17 11:53 ` Sakari Ailus
@ 2014-07-17 12:13 ` Laurent Pinchart
2014-09-24 9:17 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2014-07-17 12:13 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Sakari Ailus, linux-media
On Thursday 17 July 2014 14:53:49 Sakari Ailus wrote:
> On Thu, Jul 17, 2014 at 01:43:09PM +0200, Laurent Pinchart wrote:
> > On Tuesday 27 May 2014 16:27:49 Sakari Ailus wrote:
> > > Calling media_entity_cleanup() on a cleaned-up entity would result into
> > > double free of the entity->links pointer and likely memory corruption as
> > > well.
> >
> > My first question is, why would anyone do that ? :-)
>
> Because it makes error handling easier. Many cleanup functions work this
> way, but not media_entity_cleanup().
Do the cleanup functions support being called multiple times, or do they just
support being called on memory that has been zeroed and not further
initialized ? The media_entity_cleanup() function supports the latter.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: Set entity->links NULL in cleanup
2014-07-17 12:13 ` Laurent Pinchart
@ 2014-09-24 9:17 ` Sakari Ailus
2014-09-26 14:20 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2014-09-24 9:17 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus; +Cc: linux-media
Hi Laurent,
Oops. this got buried in my inbox...
Laurent Pinchart wrote:
> On Thursday 17 July 2014 14:53:49 Sakari Ailus wrote:
>> On Thu, Jul 17, 2014 at 01:43:09PM +0200, Laurent Pinchart wrote:
>>> On Tuesday 27 May 2014 16:27:49 Sakari Ailus wrote:
>>>> Calling media_entity_cleanup() on a cleaned-up entity would result into
>>>> double free of the entity->links pointer and likely memory corruption as
>>>> well.
>>>
>>> My first question is, why would anyone do that ? :-)
>>
>> Because it makes error handling easier. Many cleanup functions work this
>> way, but not media_entity_cleanup().
>
> Do the cleanup functions support being called multiple times, or do they just
> support being called on memory that has been zeroed and not further
> initialized ? The media_entity_cleanup() function supports the latter.
I'd hope they wouldn't be called multiple times, or on memory that's not
been zeroed, but in that case it's better to behave rather than corrupt
system memory. That could be an indication of other problems, too, so
one could consider adding WARN_ON() to this as well. What do you think?
--
Kind regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: Set entity->links NULL in cleanup
2014-09-24 9:17 ` Sakari Ailus
@ 2014-09-26 14:20 ` Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2014-09-26 14:20 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Sakari Ailus, linux-media
Hi Sakari,
On Wednesday 24 September 2014 12:17:45 Sakari Ailus wrote:
> Hi Laurent,
>
> Oops. this got buried in my inbox...
>
> Laurent Pinchart wrote:
> > On Thursday 17 July 2014 14:53:49 Sakari Ailus wrote:
> >> On Thu, Jul 17, 2014 at 01:43:09PM +0200, Laurent Pinchart wrote:
> >>> On Tuesday 27 May 2014 16:27:49 Sakari Ailus wrote:
> >>>> Calling media_entity_cleanup() on a cleaned-up entity would result into
> >>>> double free of the entity->links pointer and likely memory corruption
> >>>> as well.
> >>>
> >>> My first question is, why would anyone do that ? :-)
> >>
> >> Because it makes error handling easier. Many cleanup functions work this
> >> way, but not media_entity_cleanup().
> >
> > Do the cleanup functions support being called multiple times, or do they
> > just support being called on memory that has been zeroed and not further
> > initialized ? The media_entity_cleanup() function supports the latter.
>
> I'd hope they wouldn't be called multiple times, or on memory that's not
> been zeroed, but in that case it's better to behave rather than corrupt
> system memory. That could be an indication of other problems, too, so
> one could consider adding WARN_ON() to this as well. What do you think?
I agree that calling the cleanup function on uninitialized memory simplifies
error paths, that's a good feature. Regarding double calls, I have no strong
opinion. I don't think they should happen in the first place though.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-26 14:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-27 13:27 [PATCH 1/1] media: Set entity->links NULL in cleanup Sakari Ailus
2014-07-17 11:43 ` Laurent Pinchart
2014-07-17 11:53 ` Sakari Ailus
2014-07-17 12:13 ` Laurent Pinchart
2014-09-24 9:17 ` Sakari Ailus
2014-09-26 14:20 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.