diff mbox series

[FFmpeg-devel,2/3] avformat/matroskadec: Don't discard valid packets

Message ID 20200326004144.26758-2-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/3] avformat/matroskadec: Don't discard the upper 32bits of TrackNumber
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 26, 2020, 12:41 a.m. UTC
A Block (meaning both a Block in a BlockGroup as well as a SimpleBlock)
must have at least three bytes after the field containing the encoded
TrackNumber. So a if there are <= 3 bytes, the Matroska demuxer would
skip this block, believing it to be an empty, but valid Block.

This might discard valid Blocks, even nonempty ones (namely if the track
uses header stripping). And certain definitely spec-incompliant Blocks
don't raise errors: Those with two or less bytes left after the encoded
TrackNumber and those with three bytes left, but with flags indicating
that the Block uses lacing (in which case there has to be further data
describing the lacing).

Furthermore, zero-sized packets were still possible because only the
size of the last entry of a lace was checked.

This commit fixes this. All spec-compliant Blocks are now returned to
the caller, even those with a size of zero. This is in accordance with the
documentation of av_read_frame(): "This function returns what is stored
in the file, and does not validate that what is there are valid frames
for the decoder".

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
mkclean can produce blocks where the payload has a size of zero before
readding the removed header. E.g. it does this if a stream only has one
frame in total (although in such a case the overhead of header removal
compression is greater than the number of bytes saved in the Blocks);
this can in particular happen with forced subtitle tracks with only one
frame (which is how I noticed this).

I am unsure what to do with size-zero packets; I don't know any
Matroska Codec Mapping where this would be allowed. But there is nothing
explicitly stating that they are generally illegal, so this commit
returns them. The only thing I am certain is that stripping these
packets away needs to happen after the content compressions have been
reversed.

 libavformat/matroskadec.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Anton Khirnov March 26, 2020, 2:18 p.m. UTC | #1
Quoting Andreas Rheinhardt (2020-03-26 01:41:43)
> A Block (meaning both a Block in a BlockGroup as well as a SimpleBlock)
> must have at least three bytes after the field containing the encoded
> TrackNumber. So a if there are <= 3 bytes, the Matroska demuxer would
> skip this block, believing it to be an empty, but valid Block.
> 
> This might discard valid Blocks, even nonempty ones (namely if the track
> uses header stripping). And certain definitely spec-incompliant Blocks
> don't raise errors: Those with two or less bytes left after the encoded
> TrackNumber and those with three bytes left, but with flags indicating
> that the Block uses lacing (in which case there has to be further data
> describing the lacing).
> 
> Furthermore, zero-sized packets were still possible because only the
> size of the last entry of a lace was checked.
> 
> This commit fixes this. All spec-compliant Blocks are now returned to
> the caller, even those with a size of zero. This is in accordance with the
> documentation of av_read_frame(): "This function returns what is stored
> in the file, and does not validate that what is there are valid frames
> for the decoder".
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> mkclean can produce blocks where the payload has a size of zero before
> readding the removed header. E.g. it does this if a stream only has one
> frame in total (although in such a case the overhead of header removal
> compression is greater than the number of bytes saved in the Blocks);
> this can in particular happen with forced subtitle tracks with only one
> frame (which is how I noticed this).
> 
> I am unsure what to do with size-zero packets; I don't know any
> Matroska Codec Mapping where this would be allowed. But there is nothing
> explicitly stating that they are generally illegal, so this commit
> returns them. The only thing I am certain is that stripping these
> packets away needs to happen after the content compressions have been
> reversed.

Maybe they are not illegal in matroska, but I have strong doubts that
they would be illegal to export from a demuxer. We do have packets with
no actual data (only side data), but those still contain _something_.
I suspect many users wouldn't handle outright empty packets well.
diff mbox series

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 6425973954..7aea13dda0 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2991,7 +2991,9 @@  static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf,
         return 0;
     }
 
-    av_assert0(size > 0);
+    if (size <= 0)
+        return AVERROR_INVALIDDATA;
+
     *laces    = *data + 1;
     data     += 1;
     size     -= 1;
@@ -3017,7 +3019,7 @@  static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf,
                     break;
             }
         }
-        if (size <= total) {
+        if (size < total) {
             return AVERROR_INVALIDDATA;
         }
 
@@ -3064,7 +3066,7 @@  static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf,
         }
         data += offset;
         size -= offset;
-        if (size <= total) {
+        if (size < total) {
             return AVERROR_INVALIDDATA;
         }
         lace_size[*laces - 1] = size - total;
@@ -3524,8 +3526,8 @@  static int matroska_parse_block(MatroskaDemuxContext *matroska, AVBufferRef *buf
         av_log(matroska->ctx, AV_LOG_INFO,
                "Invalid stream %"PRIu64"\n", num);
         return AVERROR_INVALIDDATA;
-    } else if (size <= 3)
-        return 0;
+    } else if (size < 3)
+        return AVERROR_INVALIDDATA;
     st = track->stream;
     if (st->discard >= AVDISCARD_ALL)
         return res;