Author: tommi
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/a6d985c3de0dfdaf4b70fcd031f6e1a3aa39b471
Time: Wed Jun 15 17:30:14 2016
File audio_processing_impl.cc is changed in this cl (and is part of stack frame #5, "webrtc::AudioProcessingImpl::ProcessStreamLocked"; frame #6, "webrtc::AudioProcessingImpl::ProcessStream")
Minimum distance from crash line to modified line: 8. (file: audio_processing_impl.cc, crashed on: 564, modified: 556).

@tommi: Assigning to you, request you to please take a look into it. Please help us to reassign if not with respect to your change.

Thanks.!

Henrik - can you take a look?
Looks like the problem is this (vad_core.c@381):

          tmp2_s32 = tmp_s16 * tmp1_s32;

"runtime error: signed integer overflow: 4096 * 748178 cannot be represented in type int"

Reassigning to kwiberg, our resident overflow fixer.

Yes, that's the problem: we multiply a 16-bit and a 32-bit number (both in Q12, but that doesn't matter) and expect the result to fit in 32 bits (in Q24). This operation can obviously fail unless the code ensures that the operands aren't too large.

Unfortunately, I have no idea exactly what the code is doing to prevent overflow, or how it fails. I could do something heavy-handed like saturate the offending multiplication, but that won't necessarily solve any real problem. Henrik, is there anyone who's still on the team who knows how this code works?

I doubt there is anyone around who _still_ knows the code good enough to provide meaningful input.

What are our options? Is suppressing one of them?

UBSan has several ways to suppress errors: http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#issue-suppression. The one that looks most appealing is to annotate functions with __attribute__((no_sanitize("foo"))); we could make an inline function that does an int16_t * int32_t -> int32_t multiplication while ignoring overflow.

While this has the advantage of exactly preserving the current behavior (well, hopefully---it *is* undefined behavior, so in theory the compiler could produce different results based on the phase of the moon), in case of overflow the result is garbage. Saturating may be better.

I have a fix that I'd like to test, but I'm having trouble. From what I understand I need to build "chrome" to trigger the bug, but when I do this:

  $ gn gen out/gn-ubsan '--args=use_goma=true is_debug=false is_ubsan=true'
  $ ninja -j1024 -C out/gn-ubsan chrome

I get compilation errors; a couple of these:

  ../../third_party/protobuf/src/google/protobuf/stubs/strutil.cc:984:17: runtime error: signed integer overflow: 42 * 100000000 cannot be represented in type 'int'

and a couple of these:

  gen/blink/core/inspector/protocol/Protocol.h:383:19: error: no member named 'parse' in 'v8_inspector::protocol::Runtime::API::RemoteObject'

I'm trying to follow the instructions here: https://dev.chromium.org/developers/testing/undefinedbehaviorsanitizer. Are there other instructions I should be following instead?

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/27c7b8ff115205653c84631fb2650f03b7fda33a

commit 27c7b8ff115205653c84631fb2650f03b7fda33a
Author: kwiberg <kwiberg@webrtc.org>
Date: Fri Sep 09 09:04:38 2016

VadCore: Allow signed multiplication overflow that we don't know how to fix

The right thing to do would be to ensure that the multiplication can't
overflow, but that'd be a major change bordering on a rewrite, and
would take too much time. So instead, we just instruct UBSan to look
the other way.

BUG= chromium:634834 

Review-Url: https://codereview.webrtc.org/2318083002
Cr-Commit-Position: refs/heads/master@{#14154}

[modify] https://crrev.com/27c7b8ff115205653c84631fb2650f03b7fda33a/webrtc/base/sanitizer.h
[modify] https://crrev.com/27c7b8ff115205653c84631fb2650f03b7fda33a/webrtc/common_audio/vad/vad_core.c

I'm hoping that the commit in comment #8 fixes the problem, but as I said earlier I haven't been able to reproduce the error locally.

ClusterFuzz has detected this issue as fixed in range 417559:417569.

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

Fuzzer: inferno_twister
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  WebRtcVad_CalcVad8khz
  WebRtcVad_CalcVad16khz
  WebRtcVad_Process
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=400002:400026
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=417559:417569

Minimized Testcase (0.09 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv96CJT-mVtRR0_H-gMJZtH1g7-eBQEooxtvB4OXxVp7Hqb9Yx1Jtw0w2J8K-5usqwOwfXfC0dJUXwyc8DLNrShF2ioXzz_bInDPkKPdtD1GMIDazIjU0en5MyWMRWGoLzFdoQn_iQVffiTT73a2yKTOFmT5fvA?testcase_id=5368572086059008
<script>
            navigator.mediaDevices.getUserMedia({ "audio": true})
            </script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

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

OK, looks like I got it right without being able to reproduce manually.

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