kwiberg: More iSAC problems.

OK, I can't reproduce this (the repro instructions don't seem to cover UBSan yet? who knows how to do this?), but it looks obvious what's happening: we call WebRtcSpl_AddSatW32 with two negative arguments whose sum underflow int32. But that's exactly the sort of case this function is supposed to be able to handle! Obviously, we need to fix it so that it does it without invoking undefined behavior.

Build with GYP_DEFINES=ubsan=1, did you do that?

No, the instructions I found for reproducing fuzzer bugs used gn. I tried is_ubsan=true, and that worked, sort of: it found some error in the standard library. I'm guessing I might need some blacklist or something?

Sorry for confusion, correct option should be 'is_ubsan_security=true'.

Hmm. With that option, I get no error at all when trying to reproduce... Are there complete step-by-step instructions somewhere?

Sorry for missing instruction. We've recently enabled UBSan builds, so haven't updated everything yet. I'll try to reproduce and give you an update.

Just reproduced with the following configuration:

is_debug = false
use_goma = true
use_libfuzzer = true
is_ubsan_security = true
enable_nacl = false
proprietary_codecs = true




$ ./audio_decoder_isacfix_fuzzer ./fuzz-3-audio_decoder_isacfix_fuzzer 
INFO: Seed: 1368768961
<..>/audio_decoder_isacfix_fuzzer: Running 1 inputs 1 time(s) each.
../../third_party/webrtc/common_audio/signal_processing/include/spl_inl.h:42:18: runtime error: signed integer overflow: -6596936 + -2147483648 cannot be represented in type 'int'
    #0 0x4468de in WebRtcSpl_AddSatW32 out/Release/../../third_party/webrtc/common_audio/signal_processing/include/spl_inl.h:42:18
<...>

+flim: See #12 for details on how to reproduce fuzzer+ubsan issues.

Thanks :)

Still no luck for me:

$ rm -rf out && GYP_DEFINES='use_goma=1' gclient sync
[lots of output]
$ gn gen out/Fuzzer --args='is_debug = false use_goma = true use_libfuzzer = true is_ubsan_security = true enable_nacl = false proprietary_codecs = true'
Done. Wrote 329 targets from 90 files in 370ms
$ ninja -j50 -C out/Fuzzer/ audio_decoder_isacfix_fuzzer
ninja: Entering directory `out/Fuzzer/'
[250/253] SOLINK ./libc++.so
clang: warning: argument unused during compilation: '-pthread'
[253/253] LINK ./audio_decoder_isacfix_fuzzer
$ ./out/Fuzzer/audio_decoder_isacfix_fuzzer /tmp/xx 
INFO: Seed: 560957859
./out/Fuzzer/audio_decoder_isacfix_fuzzer: Running 1 inputs 1 time(s) each.
/tmp/xx: 0 ms

Hopefully I'm making some simple mistake...

Hm, really strange. Let's check for simple mistakes:

- testcase attached, $ sha1sum fuzz-3-audio_decoder_isacfix_fuzzer 
054526d1e6756932107c8001babac5471d20803a

- you are using fresh enough sources (since crahs has been found at 2016-04-07 07:55:36, not older)

- clang is fresh enough, mine is:
$ clang -v
clang version 3.9.0 (trunk 264915)

Deleted: fuzz-3-audio_decoder_isacfix_fuzzer 8 bytes

fuzz-3-audio_decoder_isacfix_fuzzer 8 bytes View Download

Still no luck?

The testcase I'm using has the same hash as yours. Sources date from April 11.

clang -v says 3.4 (it's the one in /usr/bin/), but I thought the build didn't use the clang from the user's PATH? However, this reminded me that it's been a long time since I re-ran

  LLVM_FORCE_HEAD_REVISION=1 ./tools/clang/scripts/update.py --force-local-build --without-android

I did so, and... it almost works? I don't get the error I'm supposed to, but it reports several dozen "left shift of negative value" errors.

(Note: I disabled goma, since it complained that the local and remote compiler versions didn't match. But it should always work without using goma, right?)

Yeah, compilation with or without goma doesn't matter.

If you've got 'shift' reports, it is a good sign (I've enabled them recently for is_ubsan_security option). However strange that you still cannot get integer-overflow.




aizatsky@ and kcc@, what can we miss here? ClusterFuzz and I are able to reproduce integer-overflow, but kwiberg@ cannot.

You don't need to call update.py anymore. gclient sync should be enough.

I just did this:

$ gn gen out/Fuzzer --args='is_debug = false use_libfuzzer = true is_ubsan_security = true enable_nacl = false proprietary_codecs = true'
$ ninja -C out/Fuzzer/ audio_decoder_isacfix_fuzzer
$ out/Fuzzer/audio_decoder_isacfix_fuzzer ~/Downloads/fuzz-3-audio_decoder_isacfix_fuzzer
INFO: Seed: 3730120797
out/Fuzzer/audio_decoder_isacfix_fuzzer: Running 1 inputs 1 time(s) each.
../../third_party/webrtc/common_audio/signal_processing/include/spl_inl.h:42:18: runtime error: signed integer overflow: -6596936 + -2147483648 cannot be represented in type 'int'
    #0 0x4468de in WebRtcSpl_AddSatW32 out/Fuzzer/../../third_party/webrtc/common_audio/signal_processing/include/spl_inl.h:42:18
    #1 0x4468de in WebRtcIsacfix_AllpassFilter2FixDec16C out/Fuzzer/../../third_party/webrtc/modules/audio_coding/codecs/isac/fix/source/filterbanks.c:56
    #2 0x448486 in WebRtcIsacfix_FilterAndCombine1 out/Fuzzer/../../third_party/webrtc/modules/audio_coding/codecs/isac/fix/source/filterbanks.c:334:3
    #3 0x43703e in WebRtcIsacfix_DecodeImpl out/Fuzzer/../../third_party/webrtc/modules/audio_coding/codecs/isac/fix/source/decode.c:214:5


@kwiberg does it fail if you feed it the crasher? Or do you run it without arguments?

>> I don't get the error I'm supposed to, but it reports several dozen "left shift of negative value" errors.

I guess the "shift" errors are just hiding the overflow error from you. 
For me the overflow error comes after 36 "shift" errors. 

So, I took another stab at this. I did

$ gn gen out/Fuzzer --args='is_debug = false use_libfuzzer = true is_ubsan_security = true enable_nacl = false proprietary_codecs = true'
$ ninja -C ~/path/to/out/Fuzzer/ audio_decoder_isacfix_fuzzer
$ ./out/Fuzzer/audio_decoder_isacfix_fuzzer  bug601728 

and as before, got a whole bunch of left shift errors. Applying this CL

  https://codereview.webrtc.org/1989803002/

gets rid of all of them, leaving me with no errors at all.

In other words, I'm doing exactly what you do, and I just can't reproduce this.

Very strange. I think we already considered that version, but may be we are using different versions of clang/llvm.

Probably it is not the best idea, but if you are brave enough, you could try to run the binary I've built: https://drive.google.com/a/google.com/file/d/0B6cwSJfPBLTjNlVhdk01TXFIMm8/view?usp=sharing

I'm getting the following with it:
$ ./audio_decoder_isacfix_fuzzer ./fuzz-3-audio_decoder_isacfix_fuzzer 2>&1 | grep 'runtime error' | grep 'integer overflow' | wc -l
1

$ ./audio_decoder_isacfix_fuzzer ./fuzz-3-audio_decoder_isacfix_fuzzer 2>&1 | grep 'runtime error' | grep 'shift' | wc -l
38

Or would be better to download Build (https://storage.cloud.google.com/chromium-browser-libfuzzer/linux-release-ubsan/libfuzzer-linux-release-385711.zip) from ClusterFuzz's report (https://cluster-fuzz.appspot.com/testcase?key=5989362591858688).


One more stupid suggestion from my side - can we have different CPUs and can the reproducibility be affected by that? I guess no, since we instrument the binary, but don't know...

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

commit bca568bfc59f944335510762f47fe4414ea1273e
Author: kwiberg <kwiberg@webrtc.org>
Date: Mon May 23 11:07:00 2016

Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are supposed to handle overflow gracefully, and they probably did. But since the overflow handling depended on undefined behavior, a sufficiently smart optimizing compiler would have realized that it could just ignore the possibility of overflow and omit all the overflow handling code, leaving only the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code (tested with clang 3.9 with -O2).

BUG= chromium:601728 

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

[modify] https://crrev.com/bca568bfc59f944335510762f47fe4414ea1273e/webrtc/common_audio/signal_processing/include/spl_inl.h
[modify] https://crrev.com/bca568bfc59f944335510762f47fe4414ea1273e/webrtc/common_audio/signal_processing/signal_processing_unittest.cc

The CL in comment #24 should have fixed the bug, but as has been amply documented in this bug, I've been unable to reproduce the problem locally. Will wait and see what the fuzzer bots say before closing this bug. (They are expected to post here when they can no longer reproduce the bug, right?)

Yes, kwiberg@, exactly!

Soo.... no bots have stepped forward claiming to no longer being able to reproduce this. mmoroz@, do you have time to do another manual test?

ClusterFuzz has detected this issue as fixed in range 396407:396452.

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

Fuzzer: libfuzzer_audio_decoder_isacfix_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  WebRtcSpl_AddSatW32
  WebRtcIsacfix_AllpassFilter2FixDec16C
  WebRtcIsacfix_FilterAndCombine1
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=396407:396452

Minimized Testcase (0.01 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97bMqdQXwULDgH9zGE1H4s1TO4CBXFAZIGpUd2kRb4pdOB5NXRmM99_BmcUzU5ti4f4Yc7Yc-xlFhSj3doACVUJMtZiBpLaV_JquK7RHZw8H-zUBgegS_yE8sE0l8rRpdXB-Xs7eIbOpDXRsFL4KkC-13o5zA

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.

Well then. Closing!

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