search

daiyanbao / libyuv

Automatically exported from code.google.com/p/libyuv
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Signed int overflows in row_gcc.cc #563

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
Running webrtc tests under UBSan 
(https://www.chromium.org/developers/testing/undefinedbehaviorsanitizer) 
triggers signed-int overflows in libjingle_media_unittests:

https://build.chromium.org/p/client.webrtc.fyi/builders/Linux%20UBSan/builds/830
/steps/libjingle_media_unittest/logs/stdio

Example:

../../chromium/src/third_party/libyuv/source/row_gcc.cc:2903:25: runtime error: 
signed integer overflow: 128 * 16843009 cannot be represented in type 'int'

Original issue reported on code.google.com by pbos@google.com on 29 Jan 2016 at 10:23

GoogleCodeExporter commented 8 years ago 
[ RUN      ] LibYUVScaleTest.ScaleFrom320x240_Box
../../source/scale.cc:662:28: runtime error: index -2 out of bounds for type 
'int [2]'
filter 3 -      204 us C -       87 us OPT
[       OK ] LibYUVScaleTest.ScaleFrom320x240_Box (1 ms)
[ RUN      ] LibYUVScaleTest.ScaleTo352x288_None

[ RUN      ] LibYUVScaleTest.ScaleDownBy2_None
../../unit_test/../unit_test/unit_test.h:60:33: runtime error: signed integer 
overflow: -629197847 * 214013 cannot be represented in type 'int'
../../unit_test/../unit_test/unit_test.h:60:42: runtime error: signed integer 
overflow: 2145292826 + 2531011 cannot be represented in type 'int'
filter 0 -       12 us C -        1 us OPT
[       OK ] LibYUVScaleTest.ScaleDownBy2_None (1 ms)

Original comment by fbarch...@google.com on 1 Feb 2016 at 8:10

GoogleCodeExporter commented 8 years ago 
LIBYUV_DISABLE_ERMS=1 runyuv

[ RUN      ] LibYUVConvertTest.I400ToI420_Any
../../source/row_gcc.cc:2903:25: runtime error: signed integer overflow: 128 * 
16843009 cannot be represented in type 'int'
[       OK ] LibYUVConvertTest.I400ToI420_Any (1 ms)
[ RUN      ] LibYUVConvertTest.I400ToI420_Unaligned

Original comment by fbarch...@google.com on 1 Feb 2016 at 8:28

GoogleCodeExporter commented 8 years ago 
The following revision refers to this bug:
  https://chromium.googlesource.com/libyuv/libyuv.git/+/9e39c1f27124c0f7c7fb1b88e4012df886e13ca9

commit 9e39c1f27124c0f7c7fb1b88e4012df886e13ca9
Author: Frank Barchard <fbarchard@google.com>
Date: Mon Feb 01 20:29:04 2016

ubsan overflow fix for multiply by 0x01010101

This is an UBSan error reported by libjingle

[ RUN      ] WebRtcVideoFrameTest.ConvertToYUY2BufferStride
[000:000] (videoframe.cc:375): Validate frame passed. format: I420 bpp: 12 
size: 1280x720 bytes: 1382400 expected: 1382400 sample[0..3]: 73, 73, 73, 73
../../chromium/src/third_party/libyuv/source/row_gcc.cc:2903:25: runtime error: 
signed integer overflow: 128 * 16843009 cannot be represented in type 'int'
[8/614] WebRtcVideoFrameTest.ConvertToYUY2BufferStride returned/aborted with 
exit code 1 (32 ms)
[9/614] WebRtcVideoFrameTest.ConvertToYUY2BufferInverted (29 ms)
Note: Google Test filter = WebRtcVideoFrameTest.ConvertToYUY2BufferInverted

The source is uint8 and the multiply is by 0x01010101 to replicate the byte to 
4 bytes.
Changing the constant to 0x01010101u should avoid overflow.

R=harryjin@google.com
TBR=harryjin@google.com
BUG=libyuv:563

Review URL: https://codereview.chromium.org/1657533005 .

[modify] 
http://crrev.com/9e39c1f27124c0f7c7fb1b88e4012df886e13ca9/README.chromium
[modify] 
http://crrev.com/9e39c1f27124c0f7c7fb1b88e4012df886e13ca9/include/libyuv/version
.h
[modify] 
http://crrev.com/9e39c1f27124c0f7c7fb1b88e4012df886e13ca9/source/row_gcc.cc

Original comment by bugdroid1@chromium.org on 1 Feb 2016 at 8:29

GoogleCodeExporter commented 8 years ago 
row_gcc fixed.  unittest and scaler have issues too.

Original comment by fbarch...@google.com on 1 Feb 2016 at 11:19

GoogleCodeExporter commented 8 years ago 
The following revision refers to this bug:
  https://chromium.googlesource.com/libyuv/libyuv.git/+/0e7d2926e29eb173f88c627187174af16d1c1f51

commit 0e7d2926e29eb173f88c627187174af16d1c1f51
Author: Frank Barchard <fbarchard@google.com>
Date: Tue Feb 02 02:08:52 2016

UBSan: Add blacklist files for libyuvC standalone.

For more info, see
http://dev.chromium.org/developers/testing/undefinedbehaviorsanitizer

TESTED=Passing compilation using:
GYP_DEFINES="ubsan=1"
GYP_DEFINES="ubsan_vptr=1"

R=harryjin@google.com, pbos@webrtc.org
BUG=libyuv:563,webrtc:5124

Review URL: https://codereview.chromium.org/1659713002 .

[add] 
http://crrev.com/0e7d2926e29eb173f88c627187174af16d1c1f51/tools/ubsan/OWNERS
[add] 
http://crrev.com/0e7d2926e29eb173f88c627187174af16d1c1f51/tools/ubsan/blacklist.
txt
[add] 
http://crrev.com/0e7d2926e29eb173f88c627187174af16d1c1f51/tools/ubsan/vptr_black
list.txt

Original comment by bugdroid1@chromium.org on 2 Feb 2016 at 2:09

GoogleCodeExporter commented 8 years ago 
scaler is incorrectly flagged with ubsan, but can be reworked to avoid what 
ubsan thinks is dangerous.

unittest is not used by chromium, so lower priority.

Original comment by fbarch...@google.com on 2 Feb 2016 at 2:10

GoogleCodeExporter commented 8 years ago 
When adding UBSan bots for libyuv (I made a CL for that: 
https://codereview.chromium.org/1656193002), the unit tests will need to be 
fixed in order for it to go green. It's also important in order to catch 
regressions before they reach Chromium, assuming the unit tests are covering 
all the code being used in Chromium.

Original comment by kjellan...@google.com on 2 Feb 2016 at 6:56

GoogleCodeExporter commented 8 years ago 
I deployed UBSan and UBSan vptr trybots now. Will deploy commit bots as soon 
they're green.

A trial run gave this error for UBSan:

[ RUN      ] LibYUVBaseTest.Djb2_Test
../../unit_test/../unit_test/unit_test.h:60:33: runtime error: signed integer 
overflow: 56248274 * 214013 cannot be represented in type 'int'

(see 
https://uberchromegw.corp.google.com/i/tryserver.libyuv/builders/linux_ubsan/bui
lds/0/steps/libyuv_unittest/logs/stdio)

The UBSan vptr bot went green right away.

Original comment by kjellan...@google.com on 2 Feb 2016 at 8:23

GoogleCodeExporter commented 8 years ago 
The following revision refers to this bug:
  https://chromium.googlesource.com/libyuv/libyuv.git/+/05ed0c539c3912c9e284d6fceb209dfc6077df25

commit 05ed0c539c3912c9e284d6fceb209dfc6077df25
Author: Frank Barchard <fbarchard@google.com>
Date: Tue Feb 02 19:01:49 2016

rework scale code for ubsan

For more info on ubsan, see
http://dev.chromium.org/developers/testing/undefinedbehaviorsanitizer

TESTED=Passing compilation using:
GYP_DEFINES="ubsan=1"
GYP_DEFINES="ubsan_vptr=1"

R=harryjin@google.com, pbos@webrtc.org
BUG=libyuv:563

Review URL: https://codereview.chromium.org/1654253004 .

[modify] 
http://crrev.com/05ed0c539c3912c9e284d6fceb209dfc6077df25/source/scale.cc

Original comment by bugdroid1@chromium.org on 2 Feb 2016 at 7:02

GoogleCodeExporter commented 8 years ago 
The following revision refers to this bug:
  https://chromium.googlesource.com/libyuv/libyuv.git/+/903c91cc2e3c831748d6f6168b7a8bb98f6ab9ec

commit 903c91cc2e3c831748d6f6168b7a8bb98f6ab9ec
Author: Frank Barchard <fbarchard@google.com>
Date: Tue Feb 02 22:32:12 2016

fix for ubsan on unittest.h fastrand()

internal math of the fastrand function uses a multiply
and add that overflows a signed int.  This triggers a
ubsan failure:

../../unit_test/../unit_test/unit_test.h:60:33: runtime error: signed integer 
overflow: 56248274 * 214013 cannot be represented in type 'int'

This change casts the intermediate math to unsigned
int to avoid the overflow.

For more info on ubsan, see
http://dev.chromium.org/developers/testing/undefinedbehaviorsanitizer

TESTED=Passing compilation using:
GYP_DEFINES="ubsan=1"
GYP_DEFINES="ubsan_vptr=1"

R=harryjin@google.com, pbos@webrtc.org
BUG=libyuv:563

Review URL: https://codereview.chromium.org/1662453003 .

[modify] 
http://crrev.com/903c91cc2e3c831748d6f6168b7a8bb98f6ab9ec/README.chromium
[modify] 
http://crrev.com/903c91cc2e3c831748d6f6168b7a8bb98f6ab9ec/include/libyuv/version
.h
[modify] 
http://crrev.com/903c91cc2e3c831748d6f6168b7a8bb98f6ab9ec/unit_test/unit_test.cc
[modify] 
http://crrev.com/903c91cc2e3c831748d6f6168b7a8bb98f6ab9ec/unit_test/unit_test.h

Original comment by bugdroid1@chromium.org on 2 Feb 2016 at 10:33

GoogleCodeExporter commented 8 years ago 
fixed in r1571

Original comment by fbarch...@google.com on 2 Feb 2016 at 10:38

GoogleCodeExporter commented 8 years ago 
Great, I have deployed the UBSan trybots as default trybots now and added the 
new buildbots to the main waterfall.

Original comment by kjellan...@google.com on 3 Feb 2016 at 8:36