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

Issue 782118 link

Starred by 1 user

Issue metadata

Status: Verified
Owner: ----
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocking:
issue 777555



Sign in to add a comment

Integer-overflow in gsm_mult

Project Member Reported by ClusterFuzz, Nov 7 2017

Issue description

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

Fuzzer: libFuzzer_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  gsm_mult
  filter_value
  short_term_synth
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=423338:423416

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

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Cc: mich...@niedermayer.cc msrchandra@chromium.org tguilbert@chromium.org dalecur...@chromium.org pnangunoori@chromium.org
Components: Blink>Media
Labels: M-63 Test-Predator-Wrong
Predator and CL could not provide any possible suspects.
Using the code search for the file, “gsmdec_template.c” assigning to concern owner from GIT blame.

Suspecting Commit#
https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+/a59505ca76718549dfc51b9622e2d88cb60f33b5

@michael -- Could you please look into this issue, adding you to CC as we are unable to assign this issue to you.

Also CC'ing other devs as they are CC'd in the  Issue 703583 .

Thank You.

Comment 2 by michae...@gmx.at, Nov 7 2017

I cannot look at this as i have no access to the reports details "You (email=michaelni@gmx.at) are not authorized to access this page!"
../../third_party/ffmpeg/libavcodec/gsmdec_template.c:44:20: runtime error: signed integer overflow: 27443 * 84135 cannot be represented in type 'int'
 #0 0xbbf883 in gsm_mult third_party/ffmpeg/libavcodec/gsmdec_template.c:44:20
 #1 0xbbfb6b in filter_value third_party/ffmpeg/libavcodec/gsmdec_template.c:77:27
 #2 0xbbf6c2 in short_term_synth third_party/ffmpeg/libavcodec/gsmdec_template.c:107:18
#3 0xbbf0c0 in gsm_decode_block third_party/ffmpeg/libavcodec/gsmdec_template.c:148:5
#4 0xbbedc8 in ff_msgsm_decode_block third_party/ffmpeg/libavcodec/msgsmdec.c:35:11
#5 0xb73779 in gsm_decode_frame third_party/ffmpeg/libavcodec/gsmdec.c:96:15
#6 0x7d3974 in decode_simple_internal third_party/ffmpeg/libavcodec/decode.c:417:15
#7 0x7d3712 in decode_simple_receive_frame third_party/ffmpeg/libavcodec/decode.c:620:15
#8 0x7d0268 in decode_receive_frame_internal third_party/ffmpeg/libavcodec/decode.c:638:15
#9 0x7cfc94 in avcodec_send_packet third_party/ffmpeg/libavcodec/decode.c:678:15

Michael, thanks for checking, sorry about the sudden cc. Presumably you are just getting this reply via e-mail. I've attached the trace and repro.

@pnangunoori please don't automatically cc folks from the third party projects as they don't have access to these tools. 

Alternatively, Michael if this is something you want to be notified of more quickly of/automatically I can look into the process to see if it's possible for you to get automated access to these reports.
clusterfuzz-testcase-minimized-6216462640611328
113 bytes View Download
Blocking: 777555

Comment 5 by michae...@gmx.at, Nov 8 2017

gsmdec_template.c:44 is
    return (int)(a * (SUINT)b + (1 << 14)) >> 15;
The SUINT there should be unsigned

I see in BUILD.gn that CHECKED is defined, this is a bad idea. CHECKED is intended for codec debuging. And it does that by making overflows vissible through integer overflows. In this case here the overflow would mean that somethng in the input bitstream contains a large enough value to exceed the range of the used data types. Which is useful to know to debug a file that doesnt look correct when decoded but not usefull here

about more quick notifies, sure, but i cannot promise how quick i can look into issues. So as long as this doesnt come with the expectation that i have to look into it "ASAP". (for example ATM ive a headache and would, if this was a real issue that required real debuging work not do it ATM) 
Components: -Blink>Media Internals>Media>FFmpeg
Cc: mmoroz@chromium.org
Thanks Michael, I think that makes sense. Will chat with the folks who added CHECKED to see why it was done.

Sure no expectations of response, we'll continue to look into these. I'll look into the process and see if there are options here.
Dale, is there anything I should do?
Cc: wolenetz@chromium.org
I left a comment on https://chromium-review.googlesource.com/#/c/734324/ asking if either you or Matt remember why we decided to do enable this flag. It seemed to have something to do with when FLAC+MP4 went through security review. +wolenetz too.
Actually I guess it was before flac+mp4, came from issue 686513 and issue 640820. I think we just don't need to be setting this #define.

Comment 12 by mmoroz@google.com, Nov 9 2017

I don't recall why we had that flag. Thanks for the fix!
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/6891d931f3ed40103d62dc606ba7fd5ce2ef971c

commit 6891d931f3ed40103d62dc606ba7fd5ce2ef971c
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Nov 09 18:25:15 2017

Don't enable integer overflow for SUINT; no point.

We ship production builds with this defined to unsigned which is
well-defined. It's just noise (per ffmpeg maintainers) to have
ubsan triggering when we define SUINT to integer so that overflow
is undefined and caught. Per maintainer this is useful for codec
debugging, but not for anything else.

BUG= 782118 ,686513,640820

Change-Id: Ib51a94da52cd95a917817da59dffd049b0da9d96
Reviewed-on: https://chromium-review.googlesource.com/759698
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>

[modify] https://crrev.com/6891d931f3ed40103d62dc606ba7fd5ce2ef971c/BUILD.gn

Project Member

Comment 14 by ClusterFuzz, Nov 10 2017

ClusterFuzz has detected this issue as fixed in range 515364:515426.

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

Fuzzer: libFuzzer_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  gsm_mult
  filter_value
  short_term_synth
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=423338:423416
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=515364:515426

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

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 15 by ClusterFuzz, Nov 10 2017

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

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

Sign in to add a comment