crypto: Use av_freep instead of av_free.

Message ID 1307562903-88888-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö June 8, 2011, 7:55 p.m.
From: Etienne Buira <etienne.buira.lists@free.fr>

This fixes a potential double free.
---
The potential double free could only occur if close is
called after open returned with a failure - I thought we
never call close in such cases, but this makes the code safer,
and apparently somebody has run into such issues somewhere.

The original commit that this is a cherry-pick of actually
introduced a memory leak - this only fixes the potential
double free by preferring av_freep.

 libavformat/crypto.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Luca Barbato June 8, 2011, 8:09 p.m. | #1
On 6/8/11 2:55 PM, Martin Storsjö wrote:
> The potential double free could only occur if close is
> called after open returned with a failure - I thought we
> never call close in such cases, but this makes the code safer,
> and apparently somebody has run into such issues somewhere.

Ok.
Ronald Bultje June 8, 2011, 8:10 p.m. | #2
Hi,

On Wed, Jun 8, 2011 at 3:55 PM, Martin Storsjö <martin@martin.st> wrote:
> -    av_free(c->key);
> -    av_free(c->iv);
> +    av_freep(c->key);
> +    av_freep(c->iv);

av_freep(&...)?

Ronald
Etienne Buira June 8, 2011, 8:12 p.m. | #3
On Wed, Jun 08, 2011 at 10:55:03PM +0300, Martin Storsjö wrote:
> The original commit that this is a cherry-pick of actually
> introduced a memory leak - this only fixes the potential

What leak? If I trust libavformat/utils.c:avformat_free_context(),
av_opt_free() is called so the hunk you removed is only a small cleanup.
You're welcome to state if I'm wrong.
Martin Storsjö June 8, 2011, 8:19 p.m. | #4
On Wed, 8 Jun 2011, Ronald S. Bultje wrote:

> Hi,
> 
> On Wed, Jun 8, 2011 at 3:55 PM, Martin Storsjö <martin@martin.st> wrote:
> > -    av_free(c->key);
> > -    av_free(c->iv);
> > +    av_freep(c->key);
> > +    av_freep(c->iv);
> 
> av_freep(&...)?

Oh crap - yes, fixed locally.

// Martin
Martin Storsjö June 8, 2011, 8:37 p.m. | #5
On Wed, 8 Jun 2011, Etienne Buira wrote:

> On Wed, Jun 08, 2011 at 10:55:03PM +0300, Martin Storsjö wrote:
> > The original commit that this is a cherry-pick of actually
> > introduced a memory leak - this only fixes the potential
> 
> What leak? If I trust libavformat/utils.c:avformat_free_context(),
> av_opt_free() is called so the hunk you removed is only a small cleanup.

If that were the case, it'd be a case of mixing two different things in 
the same commit - in the original form, it looked like it was related to 
the change of av_free to av_freep, which it was clearly not.

> You're welcome to state if I'm wrong.

In the applehttp demuxer (which is the only place where the crypto 
protocol is used today, and it's not usable standalone at the moment, 
since you can't pass avoptions to protocols from the command line), the 
input is closed manually via a plain ffurl_close (since it uses a custom 
AVIOContext), where av_opt_free never is called on it. Also, even if it 
was closed as a normal AVIOContext, we only call av_opt_free on the 
AVFormatContext and its private data, not on the URLContext's private 
data.

To see it in action, try valgrinding "ffmpeg -i 
'http://albin.abo.fi/~mstorsjo/applehttp-crypt/prog_index.m3u8".

This cleanup could perhaps be done if we'd make ffurl_close call 
av_opt_free on the private data, though - that might make sense.

// Martin
Etienne Buira June 8, 2011, 8:53 p.m. | #6
On Wed, Jun 08, 2011 at 11:37:59PM +0300, Martin Storsjö wrote:
> On Wed, 8 Jun 2011, Etienne Buira wrote:
> 
> > On Wed, Jun 08, 2011 at 10:55:03PM +0300, Martin Storsjö wrote:
> > > The original commit that this is a cherry-pick of actually
> > > introduced a memory leak - this only fixes the potential
> > 
> > What leak? If I trust libavformat/utils.c:avformat_free_context(),
> > av_opt_free() is called so the hunk you removed is only a small cleanup.
> 
> If that were the case, it'd be a case of mixing two different things in 
> the same commit - in the original form, it looked like it was related to 
> the change of av_free to av_freep, which it was clearly not.

It was related to use of av_opt_free, which will try to double free if
not NULLed (and sorry for the obvious & mistake).

> > You're welcome to state if I'm wrong.
> 
> In the applehttp demuxer (which is the only place where the crypto 
> protocol is used today, and it's not usable standalone at the moment, 
> since you can't pass avoptions to protocols from the command line), the 
> input is closed manually via a plain ffurl_close (since it uses a custom 
> AVIOContext), where av_opt_free never is called on it. Also, even if it 
> was closed as a normal AVIOContext, we only call av_opt_free on the 
> AVFormatContext and its private data, not on the URLContext's private 
> data.
> 
> To see it in action, try valgrinding "ffmpeg -i 
> 'http://albin.abo.fi/~mstorsjo/applehttp-crypt/prog_index.m3u8".
> 
> This cleanup could perhaps be done if we'd make ffurl_close call 
> av_opt_free on the private data, though - that might make sense.
> 
> // Martin

OK, thanks for the explanation.
Martin Storsjö June 8, 2011, 8:57 p.m. | #7
On Wed, 8 Jun 2011, Etienne Buira wrote:

> On Wed, Jun 08, 2011 at 11:37:59PM +0300, Martin Storsjö wrote:
> > On Wed, 8 Jun 2011, Etienne Buira wrote:
> > 
> > > On Wed, Jun 08, 2011 at 10:55:03PM +0300, Martin Storsjö wrote:
> > > > The original commit that this is a cherry-pick of actually
> > > > introduced a memory leak - this only fixes the potential
> > > 
> > > What leak? If I trust libavformat/utils.c:avformat_free_context(),
> > > av_opt_free() is called so the hunk you removed is only a small cleanup.
> > 
> > If that were the case, it'd be a case of mixing two different things in 
> > the same commit - in the original form, it looked like it was related to 
> > the change of av_free to av_freep, which it was clearly not.
> 
> It was related to use of av_opt_free, which will try to double free if
> not NULLed (and sorry for the obvious & mistake).

I see, that makes sense - then this patch isn't just a safeguard but 
actually needed, if we'd add av_opt_free to the ffurl_open/close calls.

// Martin

Patch

diff --git a/libavformat/crypto.c b/libavformat/crypto.c
index 789a4d1..3e02647 100644
--- a/libavformat/crypto.c
+++ b/libavformat/crypto.c
@@ -97,8 +97,8 @@  static int crypto_open(URLContext *h, const char *uri, int flags)
 
     return 0;
 err:
-    av_free(c->key);
-    av_free(c->iv);
+    av_freep(c->key);
+    av_freep(c->iv);
     return ret;
 }