| Message ID | 20200326004144.26758-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/matroskadec: Don't discard the upper 32bits of TrackNumber | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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 --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;
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(-)