> On April 7, 2019 at 12:54 PM Andreas Rheinhardt via ffmpeg-devel > <ffmpeg-devel@ffmpeg.org> wrote: > > > Steve Lhomme: > > On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel wrote: > >> The earlier code had three flaws: > >> > >> 1. The case of an unknown-sized element inside a finite-sized element > >> (which is against the specifications) was not caught. > >> > >> 2. The error message wasn't helpful: It compared the length of the > >> child > >> with the offset of the end of the parent and claimed that the first > >> exceeds the latter, although that is not necessarily true. > >> > >> 3. Unknown-sized elements that are not parsed can't be skipped. Given > >> that according to the Matroska specifications only the segment and the > >> clusters can be of unknown-size, this is handled by not allowing any > > > > This restriction is rather new. There might be old files that use > > infinite sizes in other places. > > > Indeed there are. The file mentioned by Dale > (https://ffmpeg.org/pipermail/ffmpeg-devel/2019-February/240376.html; > https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm) > has cues where every master element (even CueTrackPositions) are > written as unknown-sized element (with length fields coded on eight > bytes). The Cues aren't referenced in a Seekhead at the beginning > though, so that FFmpeg doesn't try to read them. If they were > referenced, neither current nor patched FFmpeg would be able to > properly parse them and patched FFmpeg would furthermore give an error > message. > > When exactly has this restriction been enacted?
That would be 35e0909ef4fc34ca7cb15adba5e7c7ebf5daeacb in the Matroska specifications (2016/10/31). Before that any master element was allowed to use the unknown length feature. Now only Segment and Cluster are. Given WebM is based on the Matroska specifications on matroska.org I would assume they don't have this restriction yet. > I'll see if I find a way to support such elements (although I am not > sure whether it is worthwhile). > > >> other units to have infinite size whereas the earlier code would seek > >> back by 1 byte upon encountering an infinite-size element that ought > >> to be skipped. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@googlemail.com> > >> --- > >> libavformat/matroskadec.c | 26 ++++++++++++++++++++++---- > >> 1 file changed, 22 insertions(+), 4 deletions(-) > >> > >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > >> index 6fa324c0cc..0179e5426e 100644 > >> --- a/libavformat/matroskadec.c > >> +++ b/libavformat/matroskadec.c > >> @@ -1180,11 +1180,29 @@ static int > >> ebml_parse_elem(MatroskaDemuxContext *matroska, > >> if (matroska->num_levels > 0) { > >> MatroskaLevel *level = > >> &matroska->levels[matroska->num_levels - 1]; > >> int64_t pos = avio_tell(pb); > >> - if (level->length != EBML_UNKNOWN_LENGTH && > >> - (pos + length) > (level->start + level->length)) { > >> + > >> + if (length != EBML_UNKNOWN_LENGTH && > >> + level->length != EBML_UNKNOWN_LENGTH) { > >> + uint64_t elem_end = pos + length, > >> + level_end = level->start + level->length; > >> + > >> + if (level_end < elem_end) { > >> + av_log(matroska->ctx, AV_LOG_ERROR, > >> + "Element at 0x%"PRIx64" ending at > >> 0x%"PRIx64" exceeds " > >> + "containing master element ending at > >> 0x%"PRIx64"\n", > >> + pos, elem_end, level_end); > >> + return AVERROR_INVALIDDATA; > >> + } > >> + } else if (level->length != EBML_UNKNOWN_LENGTH) { > >> + av_log(matroska->ctx, AV_LOG_ERROR, "Unknown-sized > >> element " > >> + "at 0x%"PRIx64" inside parent with finite > >> size\n", pos); > >> + return AVERROR_INVALIDDATA; > > > > IMO there's no reason to return an error here. You can log the error > > and still parse the data, it should end up fine (if the code is done > > as such that you can never read past your parents boundaries). At > > least libebml can deal with that. > > > Ok, treating such elements as if they extended to the end of their > master element is easily doable. > > >> + } else if (length == EBML_UNKNOWN_LENGTH && id != > >> MATROSKA_ID_CLUSTER) { > >> + // According to the specifications only clusters > >> and segments > >> + // are allowed to be unknown-sized. > >> av_log(matroska->ctx, AV_LOG_ERROR, > >> - "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in > >> parent\n", > >> - length, level->start + level->length); > >> + "Found unknown-sized element other than a > >> cluster at " > >> + "0x%"PRIx64". Dropping the invalid > >> element.\n", pos); > >> return AVERROR_INVALIDDATA; > >> } > >> } > >> -- > >> 2.19.2 > >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".