New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 842528 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Integer-overflow in silk_noise_shape_quantizer_del_dec

Project Member Reported by ClusterFuzz, May 13 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=4913348639195136

Fuzzer: libFuzzer_audio_encoder_opus_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  silk_noise_shape_quantizer_del_dec
  silk_NSQ_del_dec_c
  silk_NSQ_wrapper_FLP
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=557220:557234

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4913348639195136

Issue filed automatically.

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

Comment 1 by ClusterFuzz, May 13 2018

Cc: kwiberg@webrtc.org mflodman@webrtc.org henrika@webrtc.org
Labels: ClusterFuzz-Auto-CC
Automatically adding ccs based on OWNERS file / target commit history.

If this is incorrect, please add ClusterFuzz-Wrong label.
Cc: -mflodman@webrtc.org flim@chromium.org minyue@chromium.org gustaf@chromium.org
Owner: hlundin@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by flim@chromium.org, May 16 2018

One option is to saturate the addition by using silk_ADD_SAT32 instead but it could cause performance slowdown, so doing sanity checks or capping earlier in the signal path might be a better solution.

Comment 4 by flim@chromium.org, May 24 2018

I ran a benchmark and the proposed change does not cause performance slowdown. The signal path prior seems reasonable given the sharp transition in this fake fuzzer input, so a fix at the offending line seems most appropriate. This bug can't be reproduced with upstream master, but there were some significant changes that would be better to wait for the next release before pulling in. Instead, I'll submit a small and safe local change temporarily until the next Opus version update.
Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2018

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

commit fe1a3b8ea982298cca6bfbb826b9dff0414bf901
Author: Felicia Lim <flim@chromium.org>
Date: Fri May 25 16:41:12 2018

Saturate add to avoid int overflow.

Bug:  842528 
Change-Id: Ic19806411dd784bf820a27a65abb1b6bb14517cc
Reviewed-on: https://chromium-review.googlesource.com/1072869
Reviewed-by: Henrik Andreasson <henrika@chromium.org>
Commit-Queue: Felicia Lim <flim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561908}
[modify] https://crrev.com/fe1a3b8ea982298cca6bfbb826b9dff0414bf901/third_party/opus/README.chromium
[modify] https://crrev.com/fe1a3b8ea982298cca6bfbb826b9dff0414bf901/third_party/opus/src/silk/NSQ_del_dec.c

Comment 6 by flim@chromium.org, May 26 2018

Status: Fixed (was: Assigned)
Project Member

Comment 7 by ClusterFuzz, May 26 2018

ClusterFuzz has detected this issue as fixed in range 561903:561912.

Detailed report: https://clusterfuzz.com/testcase?key=4913348639195136

Fuzzer: libFuzzer_audio_encoder_opus_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  silk_noise_shape_quantizer_del_dec
  silk_NSQ_del_dec_c
  silk_NSQ_wrapper_FLP
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=557220:557234
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=561903:561912

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4913348639195136

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

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

Comment 8 by ClusterFuzz, May 26 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 4913348639195136 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Thanks for fixing!

Sign in to add a comment