diff mbox

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

Message ID 1492696974-38350-1-git-send-email-derek.buitenhuis@gmail.com
State Accepted
Commit 8e6b9ef4733be91b32c8b7becd95124340b92334
Headers show

Commit Message

Derek Buitenhuis April 20, 2017, 2:02 p.m. UTC
The original author apparently never read the documentation for snprintf,
or even tested that the output was correct. 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@gmail.com>
---
 libavformat/matroskadec.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer April 20, 2017, 2:58 p.m. UTC | #1
On Thu, Apr 20, 2017 at 03:02:54PM +0100, Derek Buitenhuis wrote:

> The original author apparently never read the documentation for snprintf,
> or even tested that the output was correct.

This statement does not belong in a commit message.
Its insulting to whoever the author is and may not even be true.

[...]
Derek Buitenhuis April 20, 2017, 3:04 p.m. UTC | #2
On 4/20/2017 3:58 PM, Michael Niedermayer wrote:
> On Thu, Apr 20, 2017 at 03:02:54PM +0100, Derek Buitenhuis wrote:
>> The original author apparently never read the documentation for snprintf,
>> or even tested that the output was correct.

> This statement does not belong in a commit message.
> Its insulting to whoever the author is and may not even be true.

Removed. However it /definitely/ was not tested properly.

New message: 

    Output was apparently not tested for correctness. 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.

- Derek
Derek Buitenhuis April 23, 2017, 1:09 p.m. UTC | #3
On 4/20/2017 3:02 PM, Derek Buitenhuis wrote:
> ---
>  libavformat/matroskadec.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Ping.

- Derek
wm4 April 23, 2017, 3:28 p.m. UTC | #4
On Sun, 23 Apr 2017 14:09:06 +0100
Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:

> On 4/20/2017 3:02 PM, Derek Buitenhuis wrote:
> > ---
> >  libavformat/matroskadec.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)  
> 
> Ping.

I don't think any active developer cares about it. Considering it was
broken, nobody nobody did. So just push it.

On a quick look, this seems slightly questionable (why the ret==20),
but probably ok.

Possibly you could CC the original author if you want to have more eyes
on it.
Derek Buitenhuis April 23, 2017, 4:08 p.m. UTC | #5
On 4/23/2017 4:28 PM, wm4 wrote:
> Possibly you could CC the original author if you want to have more eyes
> on it.

I CC'd him on the other patch already, but I'll loop him in here.

- Derek
Vignesh Venkatasubramanian April 24, 2017, 11:43 p.m. UTC | #6
On Thu, Apr 20, 2017 at 7:02 AM, Derek Buitenhuis
<derek.buitenhuis@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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

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)) {
+            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);