diff mbox

[FFmpeg-devel,1/2] avformat/mpegts: parse sections with multiple tables

Message ID 20180509213501.49966-1-ffmpeg@tmm1.net
State Accepted
Commit 9152c1e495551535886cfd7a8d7c0a206691443e
Headers show

Commit Message

Aman Karmani May 9, 2018, 9:35 p.m. UTC 
From: Aman Gupta <aman@tmm1.net>

Fixes PMT parsing in some mpegts streams which contain
multiple tables within the PMT pid. Previously, the parser
assumed only one table was present in each packet, and discarded
the rest of the section data after attempting to parse the first
table.

A similar issue was documented in the BeyondTV software[1], which
helped me diagnose the same bug in the ffmpeg mpegts demuxer. I also
tried DVBInspector, libdvbpsi's dvbinfo, and tstools' tsinfo to
help debug. The former two properly read PMTs with multiple tables,
whereas the last has the same bug as ffmpeg.

I've created a minimal sample[2] which contains the combined PMT.
Here's what ffmpeg probe shows before and after this patch:

Before:

    Input #0, mpegts, from 'combined-pmt-tids.ts':
      Duration: 00:00:01.08, start: 4932.966167, bitrate: 741 kb/s
      Program 1
      No Program
        Stream #0:0[0xf9d]: Audio: ac3, 48000 Hz, mono, fltp, 96 kb/s
        Stream #0:1[0xf9b]: Audio: mp3, 0 channels, fltp
        Stream #0:2[0xf9c]: Unknown: none

After:

    Input #0, mpegts, from 'combined-pmt-tids.ts':
      Duration: 00:00:01.11, start: 4932.966167, bitrate: 718 kb/s
      Program 1
        Stream #0:0[0xf9b]: Video: mpeg2video ([2][0][0][0] / 0x0002), none(tv, top first), 29.97 fps, 29.97 tbr, 90k tbn, 90k tbc
        Stream #0:1[0xf9c](eng): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, 5.1(side), fltp, 384 kb/s
        Stream #0:2[0xf9d](spa): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, mono, fltp, 96 kb/s

With the patch, the PMT is parsed correctly so the streams are
created in the correct order, are associated with "Program 1",
and their codecs are set correctly.

[1] http://forums.snapstream.com/vb/showpost.php?p=343816&postcount=201
[2] https://s3.amazonaws.com/tmm1/combined-pmt-tids.ts

Signed-off-by: Aman Gupta <aman@tmm1.net>
---
 libavformat/mpegts.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer May 10, 2018, 7:42 p.m. UTC | #1 
On Wed, May 09, 2018 at 02:35:00PM -0700, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> Fixes PMT parsing in some mpegts streams which contain
> multiple tables within the PMT pid. Previously, the parser
> assumed only one table was present in each packet, and discarded
> the rest of the section data after attempting to parse the first
> table.
> 
> A similar issue was documented in the BeyondTV software[1], which
> helped me diagnose the same bug in the ffmpeg mpegts demuxer. I also
> tried DVBInspector, libdvbpsi's dvbinfo, and tstools' tsinfo to
> help debug. The former two properly read PMTs with multiple tables,
> whereas the last has the same bug as ffmpeg.
> 
> I've created a minimal sample[2] which contains the combined PMT.
> Here's what ffmpeg probe shows before and after this patch:
> 
> Before:
> 
>     Input #0, mpegts, from 'combined-pmt-tids.ts':
>       Duration: 00:00:01.08, start: 4932.966167, bitrate: 741 kb/s
>       Program 1
>       No Program
>         Stream #0:0[0xf9d]: Audio: ac3, 48000 Hz, mono, fltp, 96 kb/s
>         Stream #0:1[0xf9b]: Audio: mp3, 0 channels, fltp
>         Stream #0:2[0xf9c]: Unknown: none
> 
> After:
> 
>     Input #0, mpegts, from 'combined-pmt-tids.ts':
>       Duration: 00:00:01.11, start: 4932.966167, bitrate: 718 kb/s
>       Program 1
>         Stream #0:0[0xf9b]: Video: mpeg2video ([2][0][0][0] / 0x0002), none(tv, top first), 29.97 fps, 29.97 tbr, 90k tbn, 90k tbc
>         Stream #0:1[0xf9c](eng): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, 5.1(side), fltp, 384 kb/s
>         Stream #0:2[0xf9d](spa): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, mono, fltp, 96 kb/s
> 
> With the patch, the PMT is parsed correctly so the streams are
> created in the correct order, are associated with "Program 1",
> and their codecs are set correctly.
> 
> [1] http://forums.snapstream.com/vb/showpost.php?p=343816&postcount=201
> [2] https://s3.amazonaws.com/tmm1/combined-pmt-tids.ts
> 
> Signed-off-by: Aman Gupta <aman@tmm1.net>
> ---
>  libavformat/mpegts.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

probably ok.

The code looks a bit convoluted, so testing with a fuzzer probably wont hurt

thx

[...]
James Almer May 13, 2018, 4:30 p.m. UTC | #2 
On 5/9/2018 6:35 PM, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> Fixes PMT parsing in some mpegts streams which contain
> multiple tables within the PMT pid. Previously, the parser
> assumed only one table was present in each packet, and discarded
> the rest of the section data after attempting to parse the first
> table.
> 
> A similar issue was documented in the BeyondTV software[1], which
> helped me diagnose the same bug in the ffmpeg mpegts demuxer. I also
> tried DVBInspector, libdvbpsi's dvbinfo, and tstools' tsinfo to
> help debug. The former two properly read PMTs with multiple tables,
> whereas the last has the same bug as ffmpeg.
> 
> I've created a minimal sample[2] which contains the combined PMT.
> Here's what ffmpeg probe shows before and after this patch:
> 
> Before:
> 
>     Input #0, mpegts, from 'combined-pmt-tids.ts':
>       Duration: 00:00:01.08, start: 4932.966167, bitrate: 741 kb/s
>       Program 1
>       No Program
>         Stream #0:0[0xf9d]: Audio: ac3, 48000 Hz, mono, fltp, 96 kb/s
>         Stream #0:1[0xf9b]: Audio: mp3, 0 channels, fltp
>         Stream #0:2[0xf9c]: Unknown: none
> 
> After:
> 
>     Input #0, mpegts, from 'combined-pmt-tids.ts':
>       Duration: 00:00:01.11, start: 4932.966167, bitrate: 718 kb/s
>       Program 1
>         Stream #0:0[0xf9b]: Video: mpeg2video ([2][0][0][0] / 0x0002), none(tv, top first), 29.97 fps, 29.97 tbr, 90k tbn, 90k tbc
>         Stream #0:1[0xf9c](eng): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, 5.1(side), fltp, 384 kb/s
>         Stream #0:2[0xf9d](spa): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, mono, fltp, 96 kb/s
> 
> With the patch, the PMT is parsed correctly so the streams are
> created in the correct order, are associated with "Program 1",
> and their codecs are set correctly.
> 
> [1] http://forums.snapstream.com/vb/showpost.php?p=343816&postcount=201
> [2] https://s3.amazonaws.com/tmm1/combined-pmt-tids.ts
> 
> Signed-off-by: Aman Gupta <aman@tmm1.net>
> ---
>  libavformat/mpegts.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 0d1dda1c36..3c9f9421cb 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -391,7 +391,8 @@ static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>                                 const uint8_t *buf, int buf_size, int is_start)
>  {
>      MpegTSSectionFilter *tss = &tss1->u.section_filter;
> -    int len;
> +    uint8_t *cur_section_buf = NULL;
> +    int len, offset;
>  
>      if (is_start) {
>          memcpy(tss->section_buf, buf, buf_size);
> @@ -408,23 +409,26 @@ static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>          tss->section_index += len;
>      }
>  
> +    offset = 0;
> +    cur_section_buf = tss->section_buf;
> +    while (cur_section_buf - tss->section_buf < MAX_SECTION_SIZE && cur_section_buf[0] != 0xff) {
>      /* compute section length if possible */
> -    if (tss->section_h_size == -1 && tss->section_index >= 3) {
> -        len = (AV_RB16(tss->section_buf + 1) & 0xfff) + 3;
> +    if (tss->section_h_size == -1 && tss->section_index - offset >= 3) {
> +        len = (AV_RB16(cur_section_buf + 1) & 0xfff) + 3;
>          if (len > MAX_SECTION_SIZE)
>              return;
>          tss->section_h_size = len;
>      }
>  
>      if (tss->section_h_size != -1 &&
> -        tss->section_index >= tss->section_h_size) {
> +        tss->section_index >= offset + tss->section_h_size) {
>          int crc_valid = 1;
>          tss->end_of_section_reached = 1;
>  
>          if (tss->check_crc) {
> -            crc_valid = !av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, tss->section_buf, tss->section_h_size);
> +            crc_valid = !av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, cur_section_buf, tss->section_h_size);
>              if (tss->section_h_size >= 4)
> -                tss->crc = AV_RB32(tss->section_buf + tss->section_h_size - 4);
> +                tss->crc = AV_RB32(cur_section_buf + tss->section_h_size - 4);
>  
>              if (crc_valid) {
>                  ts->crc_validity[ tss1->pid ] = 100;
> @@ -434,10 +438,18 @@ static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>                  crc_valid = 2;
>          }
>          if (crc_valid) {
> -            tss->section_cb(tss1, tss->section_buf, tss->section_h_size);
> +            tss->section_cb(tss1, cur_section_buf, tss->section_h_size);
>              if (crc_valid != 1)
>                  tss->last_ver = -1;
>          }
> +
> +        cur_section_buf += tss->section_h_size;
> +        offset += tss->section_h_size;
> +        tss->section_h_size = -1;
> +    } else {
> +        tss->end_of_section_reached = 0;
> +        break;
> +    }
>      }
>  }

Valgrind seems to complain about this change (Conditional jump or move
depends on uninitialised value).

http://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-valgrind&time=20180513001958
diff mbox

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 0d1dda1c36..3c9f9421cb 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -391,7 +391,8 @@  static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
                                const uint8_t *buf, int buf_size, int is_start)
 {
     MpegTSSectionFilter *tss = &tss1->u.section_filter;
-    int len;
+    uint8_t *cur_section_buf = NULL;
+    int len, offset;
 
     if (is_start) {
         memcpy(tss->section_buf, buf, buf_size);
@@ -408,23 +409,26 @@  static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
         tss->section_index += len;
     }
 
+    offset = 0;
+    cur_section_buf = tss->section_buf;
+    while (cur_section_buf - tss->section_buf < MAX_SECTION_SIZE && cur_section_buf[0] != 0xff) {
     /* compute section length if possible */
-    if (tss->section_h_size == -1 && tss->section_index >= 3) {
-        len = (AV_RB16(tss->section_buf + 1) & 0xfff) + 3;
+    if (tss->section_h_size == -1 && tss->section_index - offset >= 3) {
+        len = (AV_RB16(cur_section_buf + 1) & 0xfff) + 3;
         if (len > MAX_SECTION_SIZE)
             return;
         tss->section_h_size = len;
     }
 
     if (tss->section_h_size != -1 &&
-        tss->section_index >= tss->section_h_size) {
+        tss->section_index >= offset + tss->section_h_size) {
         int crc_valid = 1;
         tss->end_of_section_reached = 1;
 
         if (tss->check_crc) {
-            crc_valid = !av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, tss->section_buf, tss->section_h_size);
+            crc_valid = !av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, cur_section_buf, tss->section_h_size);
             if (tss->section_h_size >= 4)
-                tss->crc = AV_RB32(tss->section_buf + tss->section_h_size - 4);
+                tss->crc = AV_RB32(cur_section_buf + tss->section_h_size - 4);
 
             if (crc_valid) {
                 ts->crc_validity[ tss1->pid ] = 100;
@@ -434,10 +438,18 @@  static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
                 crc_valid = 2;
         }
         if (crc_valid) {
-            tss->section_cb(tss1, tss->section_buf, tss->section_h_size);
+            tss->section_cb(tss1, cur_section_buf, tss->section_h_size);
             if (crc_valid != 1)
                 tss->last_ver = -1;
         }
+
+        cur_section_buf += tss->section_h_size;
+        offset += tss->section_h_size;
+        tss->section_h_size = -1;
+    } else {
+        tss->end_of_section_reached = 0;
+        break;
+    }
     }
 }