[FFmpeg-devel] [PATCH] ass subtitles: Fix valgrind warnings.

[Nicolas George]

Le nonidi 19 thermidor, an CCXX, Philip Langdale a écrit :
> We're now running some of this code through valgrind for the first
> time, and a few warnings showed up stemming from two problems.
> 
> 1) The ASS code assumes the subtitle header is null terminated, but
> it wasn't, and passing the size down doesn't look like fun, so I
> added a terminator

Actually, it is often more practical to work with pointer+len (or even
better: pointer+end_pointer) than with 0-terminated strings.

But this would require a big rework; I just wanted to mention it for
reference.

> 
> 2) The code wasn't freeing all of its state.
> 
> Signed-off-by: Philip Langdale <philipl at overt.org>
> ---
>  ffmpeg.c               |    3 ++-
>  libavcodec/ass_split.c |    4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 6098422..faeb2f5 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -3366,7 +3366,8 @@ static int transcode_init(void)
>              if ((ist = get_input_stream(ost)))
>                  dec = ist->st->codec;
>              if (dec && dec->subtitle_header) {
> -                ost->st->codec->subtitle_header = av_malloc(dec->subtitle_header_size);
> +                /* ASS code assumes this buffer is null terminated so add extra byte. */
> +                ost->st->codec->subtitle_header = av_mallocz(dec->subtitle_header_size + 1);

Looks harmless enough.

>                  if (!ost->st->codec->subtitle_header) {
>                      ret = AVERROR(ENOMEM);
>                      goto dump_format;
> diff --git a/libavcodec/ass_split.c b/libavcodec/ass_split.c
> index a0b7254..ea3f789 100644
> --- a/libavcodec/ass_split.c
> +++ b/libavcodec/ass_split.c
> @@ -352,8 +352,10 @@ void ff_ass_split_free(ASSSplitContext *ctx)
>  {
>      if (ctx) {
>          int i;
> -        for (i=0; i<FF_ARRAY_ELEMS(ass_sections); i++)
> +        for (i=0; i<FF_ARRAY_ELEMS(ass_sections); i++) {
>              free_section(ctx, &ass_sections[i]);
> +            av_free (ctx->field_order[i]);
> +        }

Maybe av_freep to avoid leaving a dangling pointer: that may make life
easier for someone else looking for another bug in the future.

>          av_free(ctx);
>      }
>  }

I do not know the code enough to affirm it is valid, but it seems
reasonable.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120805/7f0b5d4b/attachment.asc>