[FFmpeg-devel] [PATCH] avcodec/cbs_mpeg2: fix leak of extra_information_slice buffer in cbs_mpeg2_read_slice_header()

[Andreas Rheinhardt]

Hello,

James Almer:
> cbs_mpeg2_free_slice() calls av_buffer_unref() on extra_information_ref,
> meaning allocating with av_malloc() was not the intention.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> Couldn't find any mpeg2 sample containing these fields, so it's untested.
> The leak is obvious regardless of that.
>>  libavcodec/cbs_mpeg2_syntax_template.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c
> index 88cf453b17..672ff66141 100644
> --- a/libavcodec/cbs_mpeg2_syntax_template.c
> +++ b/libavcodec/cbs_mpeg2_syntax_template.c
> @@ -361,10 +361,11 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>              current->extra_information_length = k;
>              if (k > 0) {
>                  *rw = start;
> -                current->extra_information =
> -                    av_malloc(current->extra_information_length);
> -                if (!current->extra_information)
> +                current->extra_information_ref =
> +                    av_buffer_alloc(current->extra_information_length);
> +                if (!current->extra_information_ref)
>                      return AVERROR(ENOMEM);
> +                current->extra_information = current->extra_information_ref->data;
>                  for (k = 0; k < current->extra_information_length; k++) {
>                      xui(1, extra_bit_slice, bit, 0);
>                      xui(8, extra_information_slice[k],
> 

a) The reason you can't find any samples is probably that they don't
exist. The newest version of the standard that I have (Amendment 4 of
ITU-T H.262|ISO/IEC 13818-2 [1] -- this is a prepublished document and
should be taken with a grain of salt) contains this about this field:
"Reserved. A decoder conforming to this Specification that encounters
extra_information_slice in a bitstream shall ignore it (i.e. remove
from the bitstream and discard). A bitstream conforming to this
Specification shall not contain this syntax element."

The version from 02/2000 contains the same.

b) Shouldn't this buffer be padded? See 41ed2c38.

c) The parsing process of the picture header seems wrong, too: The
specs from 02/2000 contain an extra_information_picture construct just
like extra_information_slice (the description of the semantics of
these fields are analogouos); and the aformentioned version from 2012
now contains the possibility for content_description_data at this
place which can also be arbitrarily large (there can be arbitrarily
many content_description_data blocks, but each one can't be
arbitrarily large).

Should we implement the 02/2000 version (way simpler than the newer
one), then this means that it might make sense to do this allocation
stuff via a macro like in cbs_h2645.

d) I have already said that this pre-published version should be taken
with a grain of salt: The syntax in this version doesn't contain an
extra_information_picture element any more; but the semantics still
do. So it would be helpful if someone has the version currently in force.

- Andreas

[1]: https://www.itu.int/rec/T-REC-H.262-201202-T!Amd4/en