New issue
Advanced search Search tips

Issue 763620 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocked on:
issue 920364



Sign in to add a comment

CHECK failure: lhs < rhs. NUMBER < NUMBER in ranges.cc

Project Member Reported by ClusterFuzz, Sep 9 2017

Issue description

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

Fuzzer: libFuzzer_mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  lhs < rhs. NUMBER < NUMBER in ranges.cc
  base::debug::DebugBreak
  media::Ranges<base::TimeDelta>::DCheckLT
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=499783:499873

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

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 
Labels: Test-Predator-Wrong-CLs M-63 CF-NeedsTriage
Suspected commit is very old. Adding CF-NeedsTriage label.
Could some one please look into the issue.
Thank You.
Project Member

Comment 2 by ClusterFuzz, Oct 1 2017

Components: Internals>Media
Labels: Test-Predator-AutoComponents
Automatically applying components based on information from OWNERS files. If this seems incorrect, please apply the Test-Predator-Wrong-Components label.
Cc: chcunningham@chromium.org tguilbert@chromium.org
Owner: wolenetz@chromium.org
Status: Assigned (was: Untriaged)
Roping in tguilbert@ as well since these continue to come in. Thomas, please chat with Matt to see if you can take this one or another off his or chcunningham@'s plate. Thanks!
Components: Internals>Media>Source
Status: Started (was: Assigned)
I have a successful local repro of this. I'll take this, as it looks like it might be close to the code that my PTS/DTS work also touches (though this issue predates PTS/DTS my compliance code landing).
Labels: -Pri-1 Pri-2
Interesting - the fuzzer found a fault in FrameProcessor's implementation:

FrameProcessor only attempts to "IncreaseDurationIfNecessary" using the "group_end_timestamp" resulting from the processing of all of the most recently processed frames.

But the fuzzer found a case where that group_end_timestamp is not necessarily the highest one encountered in a single call to ProcessFrames(), where that call contained (one or more) DTS discontinuities, with some earlier ones having higher group_end_timestamp that gets reset by later one to an earlier value.
Then, when augmenting the SBS GetBufferedTime() results with an intersection range of [range(start(0), duration), where the start(0) value could be < the stale duration.

This seems P2, but does need fixing:
This fuzzer case allows an earliest range start time to be larger than duration.

Two approaches at least, to fix this DCHECK:
1) IncreaseDurationIfNecessary in FP more granularly (e.g. at every discontinuity before resetting group_end_timestamp_, not just @ end of ProcessFrames())
2) Or IncreaseDurationIfNecessary once in FP ProcessFrames(), but not with group_end_timestamp_; rather use the maximum of all streams for that FP's buffered end time.

Note that the latter approach (2) in #5 is more in alignment with the "new abort and duration truncation logic" that is in MediaSourceExperimental currently (and expected to ship sometime soon).

Comment 7 by mmoroz@chromium.org, Oct 24 2017

For more information, please see https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md.

The link referenced in the description is no longer valid.
@#6, approach #2 in #5 is, I believe, compatible with the spirit of the related line of the MSE spec (the last step of the coded frame processing algorithm):
"If the media segment contains data beyond the current duration, then run the duration change algorithm with new duration set to the maximum of the current duration and the group end timestamp", which is meant to increase the duration to the maximum frame end timestamp of any frame added to the track buffer earlier in the algorithm.

I've filed spec issue https://github.com/w3c/media-source/issues/203 to obtain more comment.


Labels: -Test-Predator-AutoComponents Test-Predator-Auto-Components
Cc: sande...@chromium.org
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 6 2017

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

commit c4c936cf14b8c7f125a6087241b7b7983644ee49
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Dec 06 02:30:27 2017

MSE: Signal SBS of new CFG more granularly when buffering ByPts

This change includes multiple fixes for various cases where
SourceBufferStream requires notification of a new coded frame group when
buffering by PTS (currently available via kMseBufferByPts feature) even
if there is no MSE coded frame processing algorithm DTS discontinuity
detected. This additional granularity allows for SourceBufferStream to
understand that the next buffers appended to it may overlap recently
appended buffers, and also allows for handling cases where the next
keyframe (continuous in DTS) jumps significantly forward in PTS but
still needs to remain continuous with the current append sequence.

On every keyframe, when buffering by PTS, FrameProcessor now considers
doing the extra signalling if DTS is continuous and buffering ByPts,
and:
a) keyframe PTS jumps significantly into the future relative to the
   highest PTS emitted already in the current coded frame group (in
   which case the highest PTS emitted already in the current CFG is used
   as the signalled PTS value), or
b) keyframe PTS is before the highest PTS emitted already in the current
   coded frame group (in which case the keyframe PTS is used as the
   signalled PTS value and tracking of the highest PTS emitted is reset
   to enable correct detection and signalling of both (a) and (b) cases
   for future keyframes in the continuous DTS append sequence.)

This change also includes a fix to SourceBufferStream when buffering
ByPts to appropriately split a range that is being overlap-appended.

A benign DCHECK is also removed from SBS::UpdateLastAppendStateForRemove
which could fail for a SAP Type 2 GOP at the beginning of a range (the
last appended buffer in that range might indeed be a non-keyframe with a
PTS prior to the range start time, which could be as late as the PTS of
the keyframe of that non-keyframe's GOP.)

New and updated unit tests are included. I locally confirmed this fixes
bugs  773115  and  788344 , and appears to fix nest.com and twitch.com
renderer crashes with dcheck_always_on=true.

Pre-existing  bug 791095  is demonstrated, but not yet fixed, by new
unit tests:
  BufferingByPts_ContinuousDts_SapType2_and_PtsJumpForward
  BufferingByPts_ContinuousDts_NewSap2GopEndOverlapsLastGop_1

Pre-existing bug 763620 is demonstrated, but not yet fixed, by new
unit tests:
  BufferingByPts_ContinuousDts_NewGopEndOverlapsLastGop_2
  BufferingByPts_ContinuousDts_NewSap2GopEndOverlapsLastGop_2
  BufferingByPts_ContinuousDts_GopKeyframePtsOrder_2_1_3

BUG= 773115 , 788344 , 791095 ,763620

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I6f95cf85e1d1fa5b5f74ed1d99a3853ec6ccf686
Reviewed-on: https://chromium-review.googlesource.com/777778
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521961}
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/base/test_helpers.h
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/chunk_demuxer_unittest.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/frame_processor.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 12 by ClusterFuzz, Dec 1

Labels: -Reproducible Unreproducible
ClusterFuzz testcase 5476929955430400 appears to be flaky, updating reproducibility label.
Labels: -Unreproducible Reproducible
Please ignore the last comment about testcase being unreproducible. The testcase is still reproducible. This happened due to a code refactoring on ClusterFuzz side, and the underlying root cause is now fixed. Resetting the label back to Reproducible. Sorry about the inconvenience caused from these incorrect notifications.
Please ignore the last comment about testcase being unreproducible. The testcase is still reproducible. This happened due to a code refactoring on ClusterFuzz side, and the underlying root cause is now fixed. Resetting the label back to Reproducible. Sorry about the inconvenience caused from these incorrect notifications.
Project Member

Comment 15 by ClusterFuzz, Jan 9

ClusterFuzz has detected this issue as fixed in range 621021:621022.

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

Fuzzer: libFuzzer_mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Fuzz target binary: mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  lhs < rhs. NUMBER < NUMBER in ranges.cc
  media::Ranges<base::TimeDelta>::DCheckLT
  media::Ranges<base::TimeDelta>::Add
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=499783:499873
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=621021:621022

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

See https://github.com/google/clusterfuzz-tools for instructions to reproduce this bug locally.

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

Comment 16 by ClusterFuzz, Jan 9

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

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Cc: mmoroz@chromium.org w...@chromium.org
Hmm, this is not fixed, it just took time to hit the timeout and there's now a default timeout in the runloop. See d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6 -- +wez, mmoroz
Blockedon: 920364
Status: Assigned (was: Verified)

Sign in to add a comment