From 1f795b6172a5b3d31f20debe46531482e7a033d6 Mon Sep 17 00:00:00 2001
From: wm4 <wm4@nowhere>
Date: Mon, 12 Jan 2015 02:11:51 +0100
Subject: demux_mkv: better check for some EBML parsing

Apparently, originally this code was meant to be able to read past the
buffer somewhat, which is why the buffer allocation was padded by 8
byte. This is unclean and confuses valgrind. This probably could have
crashed with certain invalid files too.

Also revert the change added with 10a2f69; it should be not needed
anymore.
---
 demux/ebml.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/demux/ebml.c b/demux/ebml.c
index d94aed5f64..946526bee6 100644
--- a/demux/ebml.c
+++ b/demux/ebml.c
@@ -279,8 +279,11 @@ static uint32_t ebml_parse_id(uint8_t *data, int *length)
     return id;
 }
 
-static uint64_t parse_vlen(uint8_t *data, int *length, bool is_length)
+static uint64_t ebml_parse_length(uint8_t *data, size_t data_len, int *length)
 {
+    uint8_t *end = data + data_len;
+    if (data == end)
+        return -1;
     uint64_t r = *data++;
     int len = 1;
     int len_mask;
@@ -297,11 +300,13 @@ static uint64_t parse_vlen(uint8_t *data, int *length, bool is_length)
     if (r == len_mask - 1)
         num_allones++;
     for (int i = 1; i < len; i++) {
+        if (data == end)
+            return -1;
         if (*data == 255)
             num_allones++;
         r = (r << 8) | *data++;
     }
-    if (is_length && num_allones == len) {
+    if (num_allones == len) {
         // According to Matroska specs this means "unknown length"
         // Could be supported if there are any actual files using it
         *length = -1;
@@ -311,11 +316,6 @@ static uint64_t parse_vlen(uint8_t *data, int *length, bool is_length)
     return r;
 }
 
-static uint64_t ebml_parse_length(uint8_t *data, int *length)
-{
-    return parse_vlen(data, length, true);
-}
-
 static uint64_t ebml_parse_uint(uint8_t *data, int length)
 {
     assert(length >= 1 && length <= 8);
@@ -371,7 +371,7 @@ static void ebml_parse_element(struct ebml_parse_ctx *ctx, void *target,
             goto other_error;
         }
         p += len;
-        uint64_t length = ebml_parse_length(p, &len);
+        uint64_t length = ebml_parse_length(p, end - p, &len);
         if (len > end - p)
             goto past_end_error;
         if (len < 0) {
@@ -459,10 +459,16 @@ static void ebml_parse_element(struct ebml_parse_ctx *ctx, void *target,
     while (data < end) {
         int len;
         uint32_t id = ebml_parse_id(data, &len);
-        assert(len >= 0 && len <= end - data);
+        if (len < 0 || len > end - data) {
+            MP_DBG(ctx, "Error parsing subelement\n");
+            break;
+        }
         data += len;
-        uint64_t length = ebml_parse_length(data, &len);
-        assert(len >= 0 && len <= end - data);
+        uint64_t length = ebml_parse_length(data, end - data, &len);
+        if (len < 0 || len > end - data) {
+            MP_DBG(ctx, "Error parsing subelement length\n");
+            break;
+        }
         data += len;
         if (length > end - data) {
             // Try to parse what is possible from inside this partial element
@@ -614,7 +620,7 @@ int ebml_read_element(struct stream *s, struct ebml_parse_ctx *ctx,
         MP_MSG(ctx, msglevel, "Refusing to read element over 100 MB in size\n");
         return -1;
     }
-    ctx->talloc_ctx = talloc_size(NULL, length + 8);
+    ctx->talloc_ctx = talloc_size(NULL, length);
     int read_len = stream_read(s, ctx->talloc_ctx, length);
     if (read_len < length)
         MP_MSG(ctx, msglevel, "Unexpected end of file - partial or corrupt file?\n");
-- 
cgit v1.2.3