[FFmpeg-devel] [PATCH 31/37] avformat/matroskadec: Improve invalid length error handling

[Andreas Rheinhardt]

1. Up until now, the error message for EBML numbers whose length exceeds
the limits imposed upon them because of the element's type did not
distinguish between known-length and unknown-length elements. As a
consequence, the numerical value of the define constant
EBML_UNKNOWN_LENGTH was emitted as part of the error message which is
of course not appropriate. This commit changes this by adding error
messages designed for unknown-length elements.

2. We impose some (arbitrary) sanity checks on the lengths of certain
element types; these checks were conducted before the checks depending
on whether the element exceeds its containing master element. Now the
order has been reversed, because a failure at the (formerly) latter
check implies that the file is truly erroneous and not only fails our
arbitrary length limit. Moreover, this increases the informativeness of
the error messages.

3. Furthermore, the error message in general has been changed by replacing
the type of the element (something internal to this demuxer and
therefore suitable as debug output at best, not as an error message
intended for ordinary users) with the element ID. The element's position
has been added, too.

4. Finally, the length limit for EBML_NONE elements has been changed so
that all unknown-length elements of EBML_NONE-type trigger an error.
This is done because unknown-length elements can't be skipped and need
to be parsed, but there is no syntax to parse available for EBML_NONE
elements. This is done in preparation for a further patch which allows
more unknown-length elements than just clusters and segments.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
 libavformat/matroskadec.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index f19e0fc523..85d8252f6d 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1165,6 +1165,8 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
                       EbmlSyntax *syntax, void *data)
 {
     static const uint64_t max_lengths[EBML_TYPE_COUNT] = {
+        // Forbid unknown-length EBML_NONE elements.
+        [EBML_NONE]  = EBML_UNKNOWN_LENGTH - 1,
         [EBML_UINT]  = 8,
         [EBML_SINT]  = 8,
         [EBML_FLOAT] = 8,
@@ -1249,12 +1251,6 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
         matroska->current_id = 0;
         if ((res = ebml_read_length(matroska, pb, &length)) < 0)
             return res;
-        if (max_lengths[syntax->type] && length > max_lengths[syntax->type]) {
-            av_log(matroska->ctx, AV_LOG_ERROR,
-                   "Invalid length 0x%"PRIx64" > 0x%"PRIx64" for syntax element %i\n",
-                   length, max_lengths[syntax->type], syntax->type);
-            return AVERROR_INVALIDDATA;
-        }
 
         pos_alt += res;
 
@@ -1293,6 +1289,26 @@ static int ebml_parse(MatroskaDemuxContext *matroska,
         } else
             level_check = 0;
 
+        if (max_lengths[syntax->type] && length > max_lengths[syntax->type]) {
+            if (length != EBML_UNKNOWN_LENGTH) {
+                av_log(matroska->ctx, AV_LOG_ERROR,
+                       "Invalid length 0x%"PRIx64" > 0x%"PRIx64" for element "
+                       "with ID 0x%"PRIX32" at 0x%"PRIx64"\n",
+                       length, max_lengths[syntax->type], id, pos);
+            } else if (syntax->type != EBML_NONE) {
+                av_log(matroska->ctx, AV_LOG_ERROR,
+                       "Element with ID 0x%"PRIX32" at pos. 0x%"PRIx64" has "
+                       "unknown length, yet the length of an element of its "
+                       "type must be known.\n", id, pos);
+            } else {
+                av_log(matroska->ctx, AV_LOG_ERROR,
+                       "Found unknown-length element with ID 0x%"PRIX32" at "
+                       "pos. 0x%"PRIx64" for which no syntax for parsing is "
+                       "available.\n", id, pos);
+            }
+            return AVERROR_INVALIDDATA;
+        }
+
         if (!(pb->seekable & AVIO_SEEKABLE_NORMAL)) {
             // Loosing sync will likely manifest itself as encountering unknown
             // elements which are not reliably distinguishable from elements
-- 
2.21.0