[FFmpeg-devel] aacenc: Free any extradata before re-allocating.

Submitted by Josh Allmann on Feb. 6, 2018, 6:56 a.m.

Details

Message ID 1517900195-1879-1-git-send-email-joshua.allmann@gmail.com
State New
Headers show

Commit Message

Josh Allmann Feb. 6, 2018, 6:56 a.m.
Fixes a leak that occurs if avctx->extradata contains any data
prior to opening the codec, eg left over from an initialization
call to avcodec_parameters_from_context.
---
 libavcodec/aacenc.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Rostislav Pehlivanov Feb. 6, 2018, 11:16 a.m.
On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann@gmail.com> wrote:

> Fixes a leak that occurs if avctx->extradata contains any data
> prior to opening the codec, eg left over from an initialization
> call to avcodec_parameters_from_context.
> ---
>  libavcodec/aacenc.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> index 6d94c76905..f8fbe69d87 100644
> --- a/libavcodec/aacenc.c
> +++ b/libavcodec/aacenc.c
> @@ -98,6 +98,10 @@ static int put_audio_specific_config(AVCodecContext
> *avctx)
>      int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1 :
> 0));
>      const int max_size = 32;
>
> +    if (avctx->extradata) {
> +        av_freep(&avctx->extradata);
> +        avctx->extradata_size = 0;
> +    }
>      avctx->extradata = av_mallocz(max_size);
>      if (!avctx->extradata)
>          return AVERROR(ENOMEM);
> --
> 2.14.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

No, its not up to the encoder to free up the extradata. Its up to the API
user to close the avctx for the encoder which will free the extradata, even
if encoder init fails. Besides, if you don't, you'll have a dirty context
from the previous encoder since they don't have to set the same avctx
fields.
Josh Allmann Feb. 6, 2018, 4:40 p.m.
On 6 February 2018 at 03:16, Rostislav Pehlivanov <atomnuker@gmail.com>
wrote:

> On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann@gmail.com>
> wrote:
>
> > Fixes a leak that occurs if avctx->extradata contains any data
> > prior to opening the codec, eg left over from an initialization
> > call to avcodec_parameters_from_context.
> > ---
> >  libavcodec/aacenc.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> > index 6d94c76905..f8fbe69d87 100644
> > --- a/libavcodec/aacenc.c
> > +++ b/libavcodec/aacenc.c
> > @@ -98,6 +98,10 @@ static int put_audio_specific_config(AVCodecContext
> > *avctx)
> >      int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1
> :
> > 0));
> >      const int max_size = 32;
> >
> > +    if (avctx->extradata) {
> > +        av_freep(&avctx->extradata);
> > +        avctx->extradata_size = 0;
> > +    }
> >      avctx->extradata = av_mallocz(max_size);
> >      if (!avctx->extradata)
> >          return AVERROR(ENOMEM);
> > --
> > 2.14.2
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> No, its not up to the encoder to free up the extradata. Its up to the API
> user to close the avctx for the encoder which will free the extradata, even
> if encoder init fails. Besides, if you don't, you'll have a dirty context
> from the previous encoder since they don't have to set the same avctx
> fields.
>

While many (all?) other encoders share the pattern of allocating extradata
without checking for previous allocations, the abstraction seems... leaky?
(Pun fully intended.) If the encoder has its avctx fields set by
avcodec_parameters_to_context, then the extradata is deep-copied. Even when
deep copying, we do take care to deallocate any existing avctx extradata,
to avoid introducing the same type of leak. Without this patch, the API
user would have to explicitly undo the work that
avcodec_parameters_to_context is trying to help with. Hence, the 'right'
workflow when using avcodec_parameters_to_context would be:

0. Allocate codec context
1. Copy codec params to context with avcodec_parameters_to_context
2. Check if extradata exists in context and deallocate from context if so.
3. Open codec.
...
4. Close codec.

These semantics don't seem clean to me, and I think we should strive to
avoid making the user deal with corner cases like these explicitly. If not,
we should at least document it.

Josh



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Hendrik Leppkes Feb. 6, 2018, 4:55 p.m.
On Tue, Feb 6, 2018 at 5:40 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
> If the encoder has its avctx fields set by
> avcodec_parameters_to_context, then the extradata is deep-copied.

But it shouldn't have. Thats not a common pattern. What source codec
parameters would you use for an encoder? What if an encoder doesn't
actually generate any extradata (in audio, very few actually do), you
could end up writing entirely unrelated extradata (say aac extradata
with an ac3 stream), which results in all sorts of weird files.

This could be repeated for all sorts of odd fields which are not
necessarily used by every encoder. For encoding, you should allocate a
context, and set the relevant properties manually (for audio, mostly
sample rate and channel count). Everything else should remain empty.
Its not intended for another context to be copied into an encoder
context.

Every field is documented who gets to write to it, and for extradata
for encoding its the encoder, not the user, so if you write into it
for an encoding context, you are using the API wrong.

- Hendrik
wm4 Feb. 6, 2018, 5:01 p.m.
On Tue, 6 Feb 2018 08:40:20 -0800
Josh Allmann <joshua.allmann@gmail.com> wrote:

> On 6 February 2018 at 03:16, Rostislav Pehlivanov <atomnuker@gmail.com>
> wrote:
> 
> > On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann@gmail.com>
> > wrote:
> >  
> > > Fixes a leak that occurs if avctx->extradata contains any data
> > > prior to opening the codec, eg left over from an initialization
> > > call to avcodec_parameters_from_context.
> > > ---
> > >  libavcodec/aacenc.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> > > index 6d94c76905..f8fbe69d87 100644
> > > --- a/libavcodec/aacenc.c
> > > +++ b/libavcodec/aacenc.c
> > > @@ -98,6 +98,10 @@ static int put_audio_specific_config(AVCodecContext
> > > *avctx)
> > >      int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1  
> > :  
> > > 0));
> > >      const int max_size = 32;
> > >
> > > +    if (avctx->extradata) {
> > > +        av_freep(&avctx->extradata);
> > > +        avctx->extradata_size = 0;
> > > +    }
> > >      avctx->extradata = av_mallocz(max_size);
> > >      if (!avctx->extradata)
> > >          return AVERROR(ENOMEM);
> > > --
> > > 2.14.2
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >  
> >
> > No, its not up to the encoder to free up the extradata. Its up to the API
> > user to close the avctx for the encoder which will free the extradata, even
> > if encoder init fails. Besides, if you don't, you'll have a dirty context
> > from the previous encoder since they don't have to set the same avctx
> > fields.
> >  
> 
> While many (all?) other encoders share the pattern of allocating extradata
> without checking for previous allocations, the abstraction seems... leaky?
> (Pun fully intended.) If the encoder has its avctx fields set by
> avcodec_parameters_to_context, then the extradata is deep-copied. Even when
> deep copying, we do take care to deallocate any existing avctx extradata,
> to avoid introducing the same type of leak. Without this patch, the API
> user would have to explicitly undo the work that
> avcodec_parameters_to_context is trying to help with. Hence, the 'right'
> workflow when using avcodec_parameters_to_context would be:
> 
> 0. Allocate codec context
> 1. Copy codec params to context with avcodec_parameters_to_context
> 2. Check if extradata exists in context and deallocate from context if so.
> 3. Open codec.
> ...
> 4. Close codec.
> 
> These semantics don't seem clean to me, and I think we should strive to
> avoid making the user deal with corner cases like these explicitly. If not,
> we should at least document it.

I'd say the encoder isn't supposed to get these codec params, and the
function you mentioned can't be called on the encoder.

Yes, that's a result of all those public fields on a big struct, and the
awkward sharing of the codec context struct type between encoders and
decoders.

Patch hide | download patch | download mbox

diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
index 6d94c76905..f8fbe69d87 100644
--- a/libavcodec/aacenc.c
+++ b/libavcodec/aacenc.c
@@ -98,6 +98,10 @@  static int put_audio_specific_config(AVCodecContext *avctx)
     int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1 : 0));
     const int max_size = 32;
 
+    if (avctx->extradata) {
+        av_freep(&avctx->extradata);
+        avctx->extradata_size = 0;
+    }
     avctx->extradata = av_mallocz(max_size);
     if (!avctx->extradata)
         return AVERROR(ENOMEM);