diff mbox series

[FFmpeg-devel,1/2] avcodec/tdsc: Fix tile checks

Message ID 20200716072728.25072-1-michael@niedermayer.cc
State Accepted
Commit 081e3001edb67dcd55fe0f68505df1fce667476d
Headers show
Series [FFmpeg-devel,1/2] avcodec/tdsc: Fix tile checks
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer July 16, 2020, 7:27 a.m. UTC
Fixes: out of array access
Fixes: crash.asf

Found-by: anton listov <greyfarn7@yandex.ru>
Reviewed-by: anton listov <greyfarn7@yandex.ru>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/tdsc.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Nicolas George July 16, 2020, 8:27 a.m. UTC | #1
Michael Niedermayer (12020-07-16):
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libswscale/tests/swscale.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
> index 845ced61bb..9c0b5a4b11 100644
> --- a/libswscale/tests/swscale.c
> +++ b/libswscale/tests/swscale.c
> @@ -248,7 +248,7 @@ end:
>          if (dstStride[i])
>              av_free(dst[i]);
>  
> -    return res;
> +    return res & 127;
>  }
>  
>  static void selfTest(const uint8_t * const ref[4], int refStride[4],

This looks fragile: there is no local guarantee that res is not a
multiple of 128, and changes in other parts of the code could have that
effect.

Better change the ultimate return value of main to !!res IMHO.

Regards,
Michael Niedermayer July 20, 2020, 8:58 p.m. UTC | #2
On Thu, Jul 16, 2020 at 09:27:27AM +0200, Michael Niedermayer wrote:
> Fixes: out of array access
> Fixes: crash.asf
> 
> Found-by: anton listov <greyfarn7@yandex.ru>
> Reviewed-by: anton listov <greyfarn7@yandex.ru>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/tdsc.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)

will apply

[...]
diff mbox series

Patch

diff --git a/libavcodec/tdsc.c b/libavcodec/tdsc.c
index eaea41c1f5..3617911071 100644
--- a/libavcodec/tdsc.c
+++ b/libavcodec/tdsc.c
@@ -390,7 +390,7 @@  static int tdsc_decode_tiles(AVCodecContext *avctx, int number_tiles)
     for (i = 0; i < number_tiles; i++) {
         int tile_size;
         int tile_mode;
-        int x, y, w, h;
+        int x, y, x2, y2, w, h;
         int ret;
 
         if (bytestream2_get_bytes_left(&ctx->gbc) < 4 ||
@@ -408,20 +408,19 @@  static int tdsc_decode_tiles(AVCodecContext *avctx, int number_tiles)
         bytestream2_skip(&ctx->gbc, 4); // unknown
         x = bytestream2_get_le32(&ctx->gbc);
         y = bytestream2_get_le32(&ctx->gbc);
-        w = bytestream2_get_le32(&ctx->gbc) - x;
-        h = bytestream2_get_le32(&ctx->gbc) - y;
+        x2 = bytestream2_get_le32(&ctx->gbc);
+        y2 = bytestream2_get_le32(&ctx->gbc);
 
-        if (x >= ctx->width || y >= ctx->height) {
+        if (x < 0 || y < 0 || x2 <= x || y2 <= y ||
+            x2 > ctx->width || y2 > ctx->height
+        ) {
             av_log(avctx, AV_LOG_ERROR,
-                   "Invalid tile position (%d.%d outside %dx%d).\n",
-                   x, y, ctx->width, ctx->height);
-            return AVERROR_INVALIDDATA;
-        }
-        if (x + w > ctx->width || y + h > ctx->height) {
-            av_log(avctx, AV_LOG_ERROR,
-                   "Invalid tile size %dx%d\n", w, h);
+                   "Invalid tile position (%d.%d %d.%d outside %dx%d).\n",
+                   x, y, x2, y2, ctx->width, ctx->height);
             return AVERROR_INVALIDDATA;
         }
+        w = x2 - x;
+        h = y2 - y;
 
         ret = av_reallocp(&ctx->tilebuffer, tile_size);
         if (!ctx->tilebuffer)