tomfinegan@, could you please take a look? Who may be a good owner for this?

Am I going crazy here? The fuzzer/ubsan output seems off. Are unsigned integers not 32 bits on the target? Why can't we shift 145 left by 24 bits and store it in an unsigned int when it's valid to do so with 255?

Here's why I'm confused. I'm running a clang build here and I cannot repro this using a much larger value:

int main(int argc, char* argv[]) {
  unsigned char bytes[4] = {0xFF, 0xFF, 0xFF, 0xFF};
  unsigned int val = bytes[0] << 24;
  return 0;
}

My compile commands:
clang -o /tmp/blah /tmp/tmpubs.cc --std=c++11 -fsanitize=integer -fsanitize=undefined -fsanitize=address -m32
clang -o /tmp/blah /tmp/tmpubs.cc --std=c++11 -fsanitize=integer -fsanitize=undefined -fsanitize=address

Sanity checks (changing the code above to left shift bytes[0] by 25) show that ubsan is working:
/tmp/tmpubs.cc:3:24: runtime error: left shift of 255 by 25 places cannot be represented in type 'int'

Now back to the bug. Some more info. The offending line according to the report:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx/source/libvpx/vpx_ports/mem_ops.h&l=92

It's this:
val = mem[0] << 24;

mem type:
const MAU_T          *mem = (const MAU_T *)vmem;
MAU_T is unsigned char.

val type:
unsigned MEM_VALUE_T  val;
MEM_VALUE_T is int.

Maybe I need more coffee. UBSAN confusion due to preproc stuff? Any ideas? 

It seems that UBSAN checks for the high bit set. Relevant bit of assembly:

.text:00000000007024FE loc_7024FE:                             ; CODE XREF: get_tile_buffers+1EEj
.text:00000000007024FE                 mov     rsi, [rbp+data]
.text:0000000000702502                 movzx   ebx, byte ptr [rsi]
.text:0000000000702505                 test    bl, bl
.text:0000000000702507                 mov     r15, [rbp+var_50]
.text:000000000070250B                 js      handle_ubsan_error_7027FB
.text:0000000000702511                 mov     eax, cs:cov_1D40864
.text:0000000000702517                 test    eax, eax
.text:0000000000702519                 jle     record_cov_702705
.text:000000000070251F
.text:000000000070251F loc_70251F:                             ; CODE XREF: get_tile_buffers+477j
.text:000000000070251F                 inc     cs:byte_1D41268
.text:0000000000702525
.text:0000000000702525 loc_702525:                             ; CODE XREF: get_tile_buffers+585j
.text:0000000000702525                 movzx   r12d, byte ptr [rsi+2]
.text:000000000070252A                 shl     rbx, 18h
.text:000000000070252E                 movzx   eax, byte ptr [rsi+1]
.text:0000000000702532                 shl     rax, 10h
.text:0000000000702536                 shl     r12, 8
.text:000000000070253A                 movzx   ecx, byte ptr [rsi+3]
.text:000000000070253E                 or      r12, rbx
.text:0000000000702541                 or      r12, rax
.text:0000000000702544                 or      r12, rcx

Will try to understand why such code is generated.

I just contacted Richard Smith for standard clarification. Here's his answer:

---- cut ----

> Is this kind of shift indeed an undefined behavior? Is this a false positive?

It depends on which language mode you're in. In C and C++98, it is undefined behavior. 145 * 2**24 does not fit in an int (the C / C++98 rule is that you cannot shift a 1 into the sign bit).

In C++11, this case has defined behavior, but left-shifting by 25 places would have undefined behavior (you cannot shift a 1 out of the sign bit in C++11).


> Is the value promoted to signed type somewhere?

Yes, the integral promotions are performed on the left-hand side, so we're left-shifting a value of type 'int'. (Otherwise this would be unconditionally undefined behavior because 24 is greater than the bit-width of 'unsigned char'.) 

---- end cut ----

If you compile your sample in C mode, then ubsan correctly complains:

$ cat tmpubs.cc && clang++ -o tmpubs -x c tmpubs.cc -fsanitize=integer -fsanitize=undefined && ./tmpubs
int main(int argc, char* argv[]) {
  volatile unsigned char bytes[4] = {0xFF, 0xFF, 0xFF, 0xFF};
  volatile unsigned int val = bytes[0] << 24;
  return 0;
}
tmpubs.cc:3:40: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'


The correct solution is to explicitly cast unsigned bytes to unsigned ints. 

ClusterFuzz has detected this testcase as flaky and is unable to reproduce it in the original crash revision. Skipping fixed testing check and marking it as potentially fixed.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4625337657065472

Fuzzer: libfuzzer_media_vpx_video_decoder_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  mem_get_be32_as_int
  get_tile_buffer
  get_tile_buffers
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=392658:393412

Minimized Testcase (0.07 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97atwO5TR1bi_VAy0YZtOHtPwSnGAxfSFhq3BchsVtl_flNLsL2z44pkuLPi8eDb9jLZNmFAGkhPY9dqMWLBKUkYtJp5Bik7Sobdef5kdAhSWu88auYm55L4Qb4wLiOZd8tSw87JlAmTtYq4x9d8eQW-Kq0sA

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

@aizatsky

Thanks for the detailed explanation. I thought all of chrome was compiled with C++11 enabled (i.e. --std=c++11), so this situation is something of a surprise... Not that chrome's use of C++11 excuses the issue entirely considering that the upstream code is typically compiled as C. Will look into fixing this upstream.

In general you can not compile a C code with --std=c++11 :( 

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5047822600896512

Fuzzer: libfuzzer_media_vpx_video_decoder_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  mem_get_be32_as_int
  get_tile_buffer
  get_tile_buffers
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=392658:393412

Minimized Testcase (0.07 Kb): https://cluster-fuzz.appspot.com/download/AMIfv958YtLCQIf2uLDAV9HPadK3QYz9dM3HgBEIrdxo0RLJcEv4bXRsdRuHp1ij-gcQJUM_rnPgKpIZr5hyWr6tnzTS_rSic5VvTgdIFMDqhfxNRKJlCDw6Xse-ojVMicjIjthCyk84Uz295AZcTA-n2kAOma_zmA

Filer: mmoroz

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

The following revision refers to this bug:
  https://chromium.googlesource.com/webm/libvpx/+/f1de6226176ff90552ebe7473d8a76e388685053

commit f1de6226176ff90552ebe7473d8a76e388685053
Author: Tom Finegan <tomfinegan@google.com>
Date: Fri May 27 16:16:35 2016

vpx_ports/mem_ops.h: cast the lhs of bitwise shifts of 24.

C does not allow for shifting into the sign bit of a signed
integer, and the two instances here become signed ints via
promotion. Explcitly cast them to unsigned MEM_VALUE_T to
avoid the problem.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=614648

Change-Id: I51165361a8c6cbb5c378cf7e4e0f4b80b3ad9a6e

[modify] https://crrev.com/f1de6226176ff90552ebe7473d8a76e388685053/vpx_ports/mem_ops.h

Will roll early next week. Rolling before a long weekend sets the tree on fire. :(

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d

commit dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d
Author: marpan <marpan@chromium.org>
Date: Wed Jun 01 15:55:23 2016

Roll src/third_party/libvpx/source/libvpx/ 4f774ac50..f80d8011a (12 commits).

https://chromium.googlesource.com/webm/libvpx.git/+log/4f774ac50e4d..f80d8011a014

$ git log 4f774ac50..f80d8011a --date=short --no-merges --format='%ad %ae %s'
2016-05-27 jzern acm_random,Rand9Signed: correct cast
2016-05-17 linfengz Upgrade fwht4x4_mmx() to fwht4x4_sse2() for vp9 and vp10.
2016-05-27 tomfinegan vpx_ports/mem_ops.h: cast the lhs of bitwise shifts of 24.
2016-05-19 linfengz Upgrade vpx_lpf_{vertical,horizontal}_4 mmx to sse2
2016-05-25 yaowu Convert to unsigned int before left shift
2016-05-25 marpan vp9: Add datarate test for 1 pass VBR mode.
2016-05-24 yaowu Fix comments in build_intra_predictors_high()
2016-05-25 yaowu Prevent read to invalid RefBuffer
2016-05-24 bvibber Move git version extras out of iOS shared framework bundle version
2016-05-24 jzern remove vp9_diamond_search_sad_avx.c
2016-05-24 slavarnway Code clean of sub_pixel_variance4xh -- 2
2016-05-20 jackychen vp9: Remove a redundent condition in sub-pixel filter choosing.

R=johannkoenig@google.com
BUG= 614701 ,  614648 ,  615046 

Review-Url: https://codereview.chromium.org/2027703002
Cr-Commit-Position: refs/heads/master@{#397153}

[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/DEPS
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/BUILD.gn
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/README.chromium
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/generate_gypi.sh
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/libvpx.gyp
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/libvpx_srcs.gni
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/libvpx_srcs_x86.gypi
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/libvpx_srcs_x86_64.gypi
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/libvpx_srcs_x86_64_intrinsics.gypi
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/libvpx_srcs_x86_intrinsics.gypi
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/linux/ia32/vp9_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/linux/ia32/vpx_dsp_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/linux/x64/vp9_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/linux/x64/vpx_dsp_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/mac/ia32/vp9_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/mac/ia32/vpx_dsp_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/mac/x64/vp9_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/mac/x64/vpx_dsp_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/vpx_version.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/win/ia32/vp9_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/win/ia32/vpx_dsp_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/win/x64/vp9_rtcd.h
[modify] https://crrev.com/dfb18ad9f81978e1beccc6ba528acf6d9ee9f07d/third_party/libvpx/source/config/win/x64/vpx_dsp_rtcd.h

ClusterFuzz has detected this issue as fixed in range 397093:397165.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5047822600896512

Fuzzer: libfuzzer_media_vpx_video_decoder_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  mem_get_be32_as_int
  get_tile_buffer
  get_tile_buffers
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=392658:393412
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=397093:397165

Minimized Testcase (0.07 Kb): https://cluster-fuzz.appspot.com/download/AMIfv958YtLCQIf2uLDAV9HPadK3QYz9dM3HgBEIrdxo0RLJcEv4bXRsdRuHp1ij-gcQJUM_rnPgKpIZr5hyWr6tnzTS_rSic5VvTgdIFMDqhfxNRKJlCDw6Xse-ojVMicjIjthCyk84Uz295AZcTA-n2kAOma_zmA

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot