Closed Bug 1335390 Opened 3 years ago Closed 3 years ago

[Static Analysis][Dereference before null check] In constructor MediaFormatReader::MediaFormatReader

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1399621)

Attachments

(1 file)

The Static Analysis tool Coverity detected that a null pointer deference can happen in the following instance: 

>> 786 MediaFormatReader::MediaFormatReader(AbstractMediaDecoder* aDecoder,
>> 787                                      MediaDataDemuxer* aDemuxer,
>> 788                                      VideoFrameContainer* aVideoFrameContainer)
>> 789   : MediaDecoderReader(aDecoder)
>> 790   , mAudio(this, MediaData::AUDIO_DATA,
>> 791            Preferences::GetUint("media.audio-max-decode-error", 3))
>> 792   , mVideo(this, MediaData::VIDEO_DATA,
>> 793            Preferences::GetUint("media.video-max-decode-error", 2))
>> 794   , mDemuxer(new DemuxerProxy(aDemuxer, aDecoder->AbstractMainThread()))
>> 795   , mDemuxerInitDone(false)
>> 796   , mLastReportedNumDecodedFrames(0)
>> 797   , mPreviousDecodedKeyframeTime_us(sNoPreviousDecodedKeyframe)
>> 798   , mInitDone(false)
>> 799   , mTrackDemuxersMayBlock(false)
>> 800   , mSeekScheduled(false)
>> 801   , mVideoFrameContainer(aVideoFrameContainer)
>> 802   , mDecoderFactory(new DecoderFactory(this))
>> 803 {
>> 804   MOZ_ASSERT(aDemuxer);
>> 805   MOZ_COUNT_CTOR(MediaFormatReader);
>> 806
>> 807   if (aDecoder && aDecoder->CompositorUpdatedEvent()) {

Since the |aDecoder| is null checked on line 807 the analyser thinks that a possible null value is accepting thus it asserts that a null pointer dereference can happen in the initialiser list on line 794.

Looking on how the ctor is called each the parameter that is passed as |aDeoder| is always checked against null, thus preventing the nullness to be passed. But still in order to silence the checker and to prevent future uses where we don't protect the call against null cases i would propose the attached patch.
Comment on attachment 8832014 [details]
Bug 1335390 - prevent nul pointer dereference in parameter initialisation.

https://reviewboard.mozilla.org/r/108444/#review109840

::: dom/media/MediaFormatReader.cpp:808
(Diff revision 1)
>  {
>    MOZ_ASSERT(aDemuxer);
>    MOZ_COUNT_CTOR(MediaFormatReader);
>  
>    if (aDecoder && aDecoder->CompositorUpdatedEvent()) {
> +    mDemuxer = MakeUnique<DemuxerProxy>(aDemuxer,

Here you are changing the logic.

This should be:

if (aDecoder) {
  mDemuxer = MakeUnique<DemuxerProxy>(aDemuxer, aDecoder->AbstractMainThread());

  if (aDecoder->CompositorUpdatedEvent()) {
    mCompositorUpdatedListener =
      aDecoder->CompositorUpdatedEvent()->Connect(
        mTaskQueue, this, &MediaFormatReader::NotifyCompositorUpdated);
  }
}
Attachment #8832014 - Flags: review?(amarchesini) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd429b170253
prevent nul pointer dereference in parameter initialisation. r=baku
https://hg.mozilla.org/mozilla-central/rev/dd429b170253
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1335624
Keeping in touch with the code maintainers would avoid conflicts and time wasted.

And this code is wrong too, now mDemuxer won't be initialised without the parent decoder being defined
Using the correct component helps too! ;-)

Also, please check and update the CID issue (on the Coverity website) to claim the issue and reference the corresponding Bugzilla bug -- if I had seen that, I wouldn't have opened another bug. Thanks!
Component: DOM → Audio/Video: Playback
You need to log in before you can comment on or make changes to this bug.