sierravmd: Do sanity checking of frame sizes

Message ID 1378973422-28849-1-git-send-email-martin@martin.st
State Committed
Commit 0ef1660a6365ce60ead8858936b6f3f8ea862826
Headers show

Commit Message

Martin Storsjö Sept. 12, 2013, 8:10 a.m.
Limit the size to INT_MAX/2 (for simplicify) to be sure that
size + BYTES_PER_FRAME_RECORD won't overflow.

Also factorize other existing error return paths.

Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
 libavformat/sierravmd.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Luca Barbato Sept. 12, 2013, 8:16 a.m. | #1
On 12/09/13 10:10, Martin Storsjö wrote:
> Limit the size to INT_MAX/2 (for simplicify) to be sure that
> size + BYTES_PER_FRAME_RECORD won't overflow.
> 
> Also factorize other existing error return paths.
> 

It is fine for me. If somebody wants the INT_MAX / 2 set as a
self-explaining macro or the precise check, please scream now.

lu
Diego Biurrun Sept. 12, 2013, 9:03 a.m. | #2
On Thu, Sep 12, 2013 at 11:10:22AM +0300, Martin Storsjö wrote:
> Limit the size to INT_MAX/2 (for simplicify) to be sure that

simplici_T_y

LGTM otherwise

Diego

Patch

diff --git a/libavformat/sierravmd.c b/libavformat/sierravmd.c
index 645b99b..8316388 100644
--- a/libavformat/sierravmd.c
+++ b/libavformat/sierravmd.c
@@ -88,7 +88,7 @@  static int vmd_read_header(AVFormatContext *s)
     unsigned char *raw_frame_table;
     int raw_frame_table_size;
     int64_t current_offset;
-    int i, j;
+    int i, j, ret;
     unsigned int total_frames;
     int64_t current_audio_pts = 0;
     unsigned char chunk[BYTES_PER_FRAME_RECORD];
@@ -175,15 +175,13 @@  static int vmd_read_header(AVFormatContext *s)
     raw_frame_table = av_malloc(raw_frame_table_size);
     vmd->frame_table = av_malloc((vmd->frame_count * vmd->frames_per_block + sound_buffers) * sizeof(vmd_frame));
     if (!raw_frame_table || !vmd->frame_table) {
-        av_free(raw_frame_table);
-        av_free(vmd->frame_table);
-        return AVERROR(ENOMEM);
+        ret = AVERROR(ENOMEM);
+        goto error;
     }
     if (avio_read(pb, raw_frame_table, raw_frame_table_size) !=
         raw_frame_table_size) {
-        av_free(raw_frame_table);
-        av_free(vmd->frame_table);
-        return AVERROR(EIO);
+        ret = AVERROR(EIO);
+        goto error;
     }
 
     total_frames = 0;
@@ -199,6 +197,11 @@  static int vmd_read_header(AVFormatContext *s)
             avio_read(pb, chunk, BYTES_PER_FRAME_RECORD);
             type = chunk[0];
             size = AV_RL32(&chunk[2]);
+            if (size > INT_MAX / 2) {
+                av_log(s, AV_LOG_ERROR, "Invalid frame size\n");
+                ret = AVERROR_INVALIDDATA;
+                goto error;
+            }
             if(!size && type != 1)
                 continue;
             switch(type) {
@@ -235,6 +238,11 @@  static int vmd_read_header(AVFormatContext *s)
     vmd->frame_count = total_frames;
 
     return 0;
+
+error:
+    av_free(raw_frame_table);
+    av_free(vmd->frame_table);
+    return ret;
 }
 
 static int vmd_read_packet(AVFormatContext *s,