videodsp: emulated_edge_mc: Offset the src pointer in a way matching the caller

Message ID 1378240453-28016-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Sept. 3, 2013, 8:34 p.m.
From: Michael Niedermayer <michaelni@gmx.at>

If the src_y variable is very large, the multiplication
src_y * linesize might overflow. The same multiplication is
done in the caller (e.g. mpeg_motion_internal) where both src_y
and linesize are ints, where part of this offsetting is undone
within emulated_edge_mc. If this multplication overflowed in the
caller, make sure it overflows in the same way here.

This is mostly relevant for e.g. h263+ where the motion vectors
can be unrestricted.

Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
 libavcodec/videodsp_template.c |    9 ++++++---
 libavcodec/x86/videodsp_init.c |    9 ++++++---
 2 files changed, 12 insertions(+), 6 deletions(-)

Comments

Luca Barbato Sept. 3, 2013, 10:09 p.m. | #1
On 03/09/13 22:34, Martin Storsjö wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
> 
> If the src_y variable is very large, the multiplication
> src_y * linesize might overflow. The same multiplication is
> done in the caller (e.g. mpeg_motion_internal) where both src_y
> and linesize are ints, where part of this offsetting is undone
> within emulated_edge_mc. If this multplication overflowed in the
> caller, make sure it overflows in the same way here.
> 
> This is mostly relevant for e.g. h263+ where the motion vectors
> can be unrestricted.
> 

Wouldn't make more sense using the int64 in the caller as well?

lu
Martin Storsjö Sept. 3, 2013, 10:16 p.m. | #2
On Wed, 4 Sep 2013, Luca Barbato wrote:

> On 03/09/13 22:34, Martin Storsjö wrote:
>> From: Michael Niedermayer <michaelni@gmx.at>
>> 
>> If the src_y variable is very large, the multiplication
>> src_y * linesize might overflow. The same multiplication is
>> done in the caller (e.g. mpeg_motion_internal) where both src_y
>> and linesize are ints, where part of this offsetting is undone
>> within emulated_edge_mc. If this multplication overflowed in the
>> caller, make sure it overflows in the same way here.
>> 
>> This is mostly relevant for e.g. h263+ where the motion vectors
>> can be unrestricted.
>> 
>
> Wouldn't make more sense using the int64 in the caller as well?

Possibly. There's a number of callers that should be checked though, I 
think most of them use int right now. Not sure if it's enough to just 
check it in mpegvideo, do other codecs also allow arbitrarily large motion 
vectors without erroring out earlier?

I guess the callers should rather use ptrdiff_t instead of int64_t 
explicitly, since that's what the emu_edge function does right now.

The splitting of the multplications (from (src_y + a + b) * linesize into 
src_y * linesize + (a+b)*linesize) should probably be done still, so that 
we're sure that if the src_y*linesize part still overflows, it does in the 
same fashion as elsewhere.

// Martin

Patch

diff --git a/libavcodec/videodsp_template.c b/libavcodec/videodsp_template.c
index 5d5a32f..334c27b 100644
--- a/libavcodec/videodsp_template.c
+++ b/libavcodec/videodsp_template.c
@@ -22,18 +22,21 @@ 
 #include "bit_depth_template.c"
 
 static void FUNC(ff_emulated_edge_mc)(uint8_t *buf, const uint8_t *src,
-                                      ptrdiff_t linesize,
+                                      ptrdiff_t linesize_arg,
                                       int block_w, int block_h,
                                       int src_x, int src_y, int w, int h)
 {
     int x, y;
     int start_y, start_x, end_y, end_x;
+    int linesize = linesize_arg;
 
     if (src_y >= h) {
-        src  += (h - 1 - src_y) * linesize;
+        src  -= src_y * linesize;
+        src  += (h - 1) * linesize;
         src_y = h - 1;
     } else if (src_y <= -block_h) {
-        src  += (1 - block_h - src_y) * linesize;
+        src  -= src_y * linesize;
+        src  += (1 - block_h) * linesize;
         src_y = 1 - block_h;
     }
     if (src_x >= w) {
diff --git a/libavcodec/x86/videodsp_init.c b/libavcodec/x86/videodsp_init.c
index f7fd267..688a894 100644
--- a/libavcodec/x86/videodsp_init.c
+++ b/libavcodec/x86/videodsp_init.c
@@ -37,19 +37,22 @@  extern emu_edge_core_func ff_emu_edge_core_mmx;
 extern emu_edge_core_func ff_emu_edge_core_sse;
 
 static av_always_inline void emulated_edge_mc(uint8_t *buf, const uint8_t *src,
-                                              ptrdiff_t linesize,
+                                              ptrdiff_t linesize_arg,
                                               int block_w, int block_h,
                                               int src_x, int src_y,
                                               int w, int h,
                                               emu_edge_core_func *core_fn)
 {
     int start_y, start_x, end_y, end_x, src_y_add = 0;
+    int linesize = linesize_arg;
 
     if (src_y >= h) {
-        src_y_add = h - 1 - src_y;
+        src      -= src_y * linesize;
+        src_y_add = h - 1;
         src_y     = h - 1;
     } else if (src_y <= -block_h) {
-        src_y_add = 1 - block_h - src_y;
+        src      -= src_y * linesize;
+        src_y_add = 1 - block_h;
         src_y     = 1 - block_h;
     }
     if (src_x >= w) {