[FFmpeg-devel] [PATCH] webm_dash_manifest_demuxer: Fix UB in cue timestamp string code and make it actually work

[Vignesh Venkatasubramanian]

On Thu, Apr 20, 2017 at 7:02 AM, Derek Buitenhuis
<derek.buitenhuis at gmail.com> wrote:
> The original author apparently never read the documentation for snprintf,
> or even tested that the output was correct.

I was the original author and i am sorry about the mistake.

> Passing overlapping memory
> to snprintf causes undefined behavior, and usually resulted in only the
> very last timestamp being written to metadata, and not a list at all.
>
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
> ---
>  libavformat/matroskadec.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 9adca8d..320d8bf 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -3823,6 +3823,7 @@ static int webm_dash_manifest_cues(AVFormatContext *s)
>      char *buf;
>      int64_t cues_start = -1, cues_end = -1, before_pos, bandwidth;
>      int i;
> +    int end = 0;
>
>      // determine cues start and end positions
>      for (i = 0; i < seekhead_list->nb_elem; i++)
> @@ -3868,10 +3869,17 @@ static int webm_dash_manifest_cues(AVFormatContext *s)
>      if (!buf) return -1;
>      strcpy(buf, "");
>      for (i = 0; i < s->streams[0]->nb_index_entries; i++) {
> -        snprintf(buf, (i + 1) * 20 * sizeof(char),
> -                 "%s%" PRId64, buf, s->streams[0]->index_entries[i].timestamp);
> -        if (i != s->streams[0]->nb_index_entries - 1)
> +        int ret = snprintf(buf + end, 20 * sizeof(char),
> +                           "%" PRId64, s->streams[0]->index_entries[i].timestamp);
> +        if (ret <= 0 || (ret == 20 && i ==  s->streams[0]->nb_index_entries - 1)) {

Given that you are trying to fix an issue here, can you please add a
comment as to what this if condition is doing? (why ret == 20 for
example). But up to you though.

> +            av_log(s, AV_LOG_ERROR, "timestamp too long.\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +        end += ret;
> +        if (i != s->streams[0]->nb_index_entries - 1) {
>              strncat(buf, ",", sizeof(char));
> +            end++;
> +        }
>      }
>      av_dict_set(&s->streams[0]->metadata, CUE_TIMESTAMPS, buf, 0);
>      av_free(buf);
> --
> 1.8.3.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



-- 
Vignesh