1325707 - Crash in opus_packet_get_samples_per_frame

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-8ee530aa-6ffc-4b2b-b63f-88f9c2161223.
=============================================================

Seen while looking at nightly crash data: http://bit.ly/2hh7vm9. Crashes started using the 20161223030226 build.

Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f179934df0c1bab590c558485d419c7910e41325&tochange=2785aaf276ba29fb2e1f5607d90d441fee42efb4

ni on :jya for some possible insight.

Comment 1

[Nicholas Nethercote [inactive]]

7 crashes in Nightly 20161223030226, making it the #6 top crash.

All the crashes have address 0x0, so it looks like |data| might be null.

jya is in the process of moving countries, so I'll ask kinetik as well for any suggestions.

Comment 2

[Nils Ohlmeier [:drno]]

As I don't see anything WebRTC related in the stack traces I'm assuming this is not WebRTC specific.

Comment 3

[Jean-Yves Avenard [:jya]]

At a guess, it's bug 1321076 that affected the webm demuxer (where opus is most used)

Comment 4

[Jean-Yves Avenard [:jya]]

actually, maybe it's bug 1323390 if that impacts opus in mp4.

there's no URL provided in those crash reports.

Change in bug 1323390 appears wrong to me anyway.

Comment 5

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> actually, maybe it's bug 1323390 if that impacts opus in mp4.

It checks profile in ESDS, and I believe ESDS is not exist in opus.

> 
> there's no URL provided in those crash reports.
> 
> Change in bug 1323390 appears wrong to me anyway.

As we discussed on irc, please let me know if anything is wrong to you.

Comment 6

[Matthew Gregan [:kinetik]]

(In reply to Marcia Knous [:marcia - use ni] from comment #0)
> Seen while looking at nightly crash data: http://bit.ly/2hh7vm9. Crashes
> started using the 20161223030226 build.

I see the same crash going back as far as the 20161218030213 build in 53.0a1, so the regression range may be wider than initially suggested.

There are also three similar crashes (in the last 6 months) in opus_packet_get_samples_per_frame in older versions which may or may not be related... one less likely alternate possibility to a recent regression is that a certain type of file triggers this and it just happened to spike recently.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

All the 53-and-newer crashes appear to be null; older ones look like nullptr (we have two 54 crashes this week already).

This appears to be a problem outside of Opus proper (just to note); it's being passed a null ptr (and apparently in DeDecode() it believes the number of frames in the null data block is not 0; else it would have exited earlier.)

jya?  gerald?

Comment 8

[[:philipp]]

the signature now also appeared in 52.0b2 - probably due to the uplift of bug 1320705...

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

jya, gerald - since this is in 52, we'll want to get a solution to it soon.  perhaps bug 1320705 is a hint

Comment 10

[Jean-Yves Avenard [:jya]]

Will work on it on Monday

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1325707: P1. Check returned value.

This is really just for cleanliness perspective, as it can never happen, we always check that demuxing a packet succeeded first.

Review commit: https://reviewboard.mozilla.org/r/110132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110132/

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1325707: P2. Handle OOM conditions when creating MediaRawData object.

Review commit: https://reviewboard.mozilla.org/r/110134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110134/

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1325707: P3. Fix coding style.

Fixed coding style of files encountered in P1 and P2.

Review commit: https://reviewboard.mozilla.org/r/110136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110136/

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1325707: P4. Fix coding style of MediaDataDemuxers.

Review commit: https://reviewboard.mozilla.org/r/110138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110138/

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834022 [details]
Bug 1325707: P1. Check returned value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110132/diff/1-2/

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834023 [details]
Bug 1325707: P2. Handle OOM conditions when creating MediaRawData object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110134/diff/1-2/

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834024 [details]
Bug 1325707: P3. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110136/diff/1-2/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834025 [details]
Bug 1325707: P4. Fix coding style of MediaDataDemuxers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110138/diff/1-2/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834023 [details]
Bug 1325707: P2. Handle OOM conditions when creating MediaRawData object.

https://reviewboard.mozilla.org/r/110134/#review111244

::: dom/media/ogg/OggCodecState.cpp:265
(Diff revision 2)
> +    !IsHeader(packet),
> +    "PacketOutAsMediaRawData can only be called on non-header packets");
>    RefPtr<MediaRawData> sample = new MediaRawData(packet->packet, packet->bytes);
> +  if (!sample->Data()) {
> +    // OOM.
> +    return nullptr;

Need to release packet

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8834022 [details]
Bug 1325707: P1. Check returned value.

https://reviewboard.mozilla.org/r/110132/#review111232

Comment 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8834023 [details]
Bug 1325707: P2. Handle OOM conditions when creating MediaRawData object.

https://reviewboard.mozilla.org/r/110134/#review111252

::: dom/media/ogg/OggCodecState.cpp:265
(Diff revision 2)
> +    !IsHeader(packet),
> +    "PacketOutAsMediaRawData can only be called on non-header packets");
>    RefPtr<MediaRawData> sample = new MediaRawData(packet->packet, packet->bytes);
> +  if (!sample->Data()) {
> +    // OOM.
> +    return nullptr;

Or really you should have an "AutoReleasePacket" RAII thing. ;-)
But I guess that could be a separate bug, in the meantime just releasing it is fine.

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8834024 [details]
Bug 1325707: P3. Fix coding style.

https://reviewboard.mozilla.org/r/110136/#review111254

r+ with nits:

::: dom/media/ogg/OggCodecState.h:68
(Diff revision 2)
> -  ogg_packet* PopFront() { return static_cast<ogg_packet*>(nsDeque::PopFront()); }
> -  ogg_packet* PeekFront() { return static_cast<ogg_packet*>(nsDeque::PeekFront()); }
> -  ogg_packet* Pop() { return static_cast<ogg_packet*>(nsDeque::Pop()); }
> -  ogg_packet* operator[](size_t aIndex) const
> +  ogg_packet *PopFront()
> +  {
> +    return static_cast<ogg_packet *>(nsDeque::PopFront());
> +  }
> +  ogg_packet *PeekFront()
> +  {
> +    return static_cast<ogg_packet *>(nsDeque::PeekFront());
> +  }
> +  ogg_packet *Pop() { return static_cast<ogg_packet *>(nsDeque::Pop()); }
> +  ogg_packet *operator[](size_t aIndex) const

Please put the *'s back with the types they belong to.

And fix your code formatter! Other files were correctly formatted, so maybe it thinks this particular file is C code? You may need to force it to consider all C-looking files to be C++, if that's possible.

::: dom/media/ogg/OggCodecState.h:282
(Diff revision 2)
> -  bool SetCodecSpecificConfig(MediaByteBuffer* aBuffer, OggPacketQueue& aHeaders);
> +  bool SetCodecSpecificConfig(MediaByteBuffer *aBuffer,
> +                              OggPacketQueue &aHeaders);

'type* param'
'type& param'

::: dom/media/webm/WebMDemuxer.cpp:690
(Diff revision 2)
> -            mSharedVideoTrackInfo = new SharedTrackInfo(mInfo.mVideo, ++sStreamSourceID);
> +            mSharedVideoTrackInfo =
> +                new SharedTrackInfo(mInfo.mVideo, ++sStreamSourceID);

Too far, there should only be a 2-space indent for the 2nd line.

Comment 23

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8834025 [details]
Bug 1325707: P4. Fix coding style of MediaDataDemuxers.

https://reviewboard.mozilla.org/r/110138/#review111262

The usual, y'know:

::: dom/media/ADTSDemuxer.cpp:569
(Diff revision 2)
>    FastSeek(media::TimeUnit());
>  }
>  
>  RefPtr<ADTSTrackDemuxer::SkipAccessPointPromise>
> -ADTSTrackDemuxer::SkipToNextRandomAccessPoint(const media::TimeUnit& aTimeThreshold)
> +ADTSTrackDemuxer::SkipToNextRandomAccessPoint(
> +  const media::TimeUnit &aTimeThreshold)

'type& param'

::: dom/media/MP3Demuxer.cpp:473
(Diff revision 2)
> -VerifyFrameConsistency(
> -    const FrameParser::Frame& aFrame1, const FrameParser::Frame& aFrame2) {
> +VerifyFrameConsistency(const FrameParser::Frame &aFrame1,
> +                       const FrameParser::Frame &aFrame2)

'type& param'

::: dom/media/flac/FlacDemuxer.cpp:647
(Diff revision 2)
>  {
>    static const int BUFFER_SIZE = 4096;
>  
>    // First check if we have a valid Flac start.
>    char buffer[BUFFER_SIZE];
> -  const uint8_t* ubuffer = reinterpret_cast<uint8_t*>(buffer); // only needed due to type constraints of ReadAt.
> +  const uint8_t *ubuffer = // only needed due to type constraints of ReadAt.

'type* var'

::: dom/media/fmp4/MP4Demuxer.cpp:463
(Diff revision 2)
>    return NS_OK;
>  }
>  
>  RefPtr<MP4TrackDemuxer::SkipAccessPointPromise>
> -MP4TrackDemuxer::SkipToNextRandomAccessPoint(const media::TimeUnit& aTimeThreshold)
> +MP4TrackDemuxer::SkipToNextRandomAccessPoint(
> +  const media::TimeUnit &aTimeThreshold)

'type& param'

::: dom/media/mediasource/MediaSourceDemuxer.h:109
(Diff revision 2)
>    void Reset() override;
>  
>    nsresult GetNextRandomAccessPoint(media::TimeUnit* aTime) override;
>  
> -  RefPtr<SkipAccessPointPromise> SkipToNextRandomAccessPoint(const media::TimeUnit& aTimeThreshold) override;
> +  RefPtr<SkipAccessPointPromise>
> +  SkipToNextRandomAccessPoint(const media::TimeUnit &aTimeThreshold) override;

'type& param'

::: dom/media/mediasource/MediaSourceDemuxer.h:124
(Diff revision 2)
> -  already_AddRefed<MediaRawData> GetSample(MediaResult& aError);
> +  DoSkipToNextRandomAccessPoint(const media::TimeUnit &aTimeThreadshold);
> +  already_AddRefed<MediaRawData> GetSample(MediaResult &aError);

'type& param'

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834022 [details]
Bug 1325707: P1. Check returned value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110132/diff/2-3/

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834023 [details]
Bug 1325707: P2. Handle OOM conditions when creating MediaRawData object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110134/diff/2-3/

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834024 [details]
Bug 1325707: P3. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110136/diff/2-3/

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834025 [details]
Bug 1325707: P4. Fix coding style of MediaDataDemuxers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110138/diff/2-3/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0afa9c6cd411
P1. Check returned value. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c15456c1b320
P2. Handle OOM conditions when creating MediaRawData object. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7266481cd443
P3. Fix coding style. r=gerald
https://hg.mozilla.org/integration/autoland/rev/cb7b14a913fd
P4. Fix coding style of MediaDataDemuxers. r=gerald

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0afa9c6cd411
https://hg.mozilla.org/mozilla-central/rev/c15456c1b320
https://hg.mozilla.org/mozilla-central/rev/7266481cd443
https://hg.mozilla.org/mozilla-central/rev/cb7b14a913fd

Comment 30

[Julien Cristau [:jcristau]]

Tracking for 52 as new regression.  Is this something we can consider uplifting?

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jean-Yves, is this something we should uplift to beta52 and aurora53? If yes, please nominate for uplift.

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834022 [details]
Bug 1325707: P1. Check returned value.

Approval Request Comment
[Feature/Bug causing the regression]: 1320705
[User impact if declined]: crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no. The crash rate was rather low to start with, I'm not 100% sure the fix is *the* fix. But it can't hurt
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: Need P1 and P2
[Is the change risky?]: Somewhat low
[Why is the change risky/not risky?]: We now refuse anything that contain a null data allocated, that includes what would otherwise be valid packet (like those whose packet size is 0). We'll get to know soon enough
[String changes made/needed]: none

Comment 33

[Jean-Yves Avenard [:jya]]

forgot to add, unfortunately, no one care about ogg...

Comment 34

[Gerry Chang [:gchang]]

Comment on attachment 8834022 [details]
Bug 1325707: P1. Check returned value.

A potential crash fix. Aurora53+.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/903c10ccfb7a
https://hg.mozilla.org/releases/mozilla-aurora/rev/2318c2c87a23

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 8834022 [details]
Bug 1325707: P1. Check returned value.

avoid a crash on OOM, beta52+

Comment 37

[Julien Cristau [:jcristau]]

Comment on attachment 8834023 [details]
Bug 1325707: P2. Handle OOM conditions when creating MediaRawData object.

[Triage Comment]

Comment 38

[Carsten Book [:Tomcat]]

needs rebasing for beta because part 2 fails like:

warning: conflicts while merging dom/media/webm/WebMDemuxer.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 39

[Jean-Yves Avenard [:jya]]

Attachment: P1. Check returned value. r=gerald

This is really just for cleanliness perspective, as it can never happen, we always check that demuxing a packet succeeded first.

MozReview-Commit-ID: FQ1yz16m9Ix

Comment 40

[Jean-Yves Avenard [:jya]]

Attachment: P2. Handle OOM conditions when creating MediaRawData object. r=gerald

MozReview-Commit-ID: HtkhrT36Kf4

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52f1268d8868
https://hg.mozilla.org/releases/mozilla-beta/rev/d2d15b1d6052
https://hg.mozilla.org/releases/mozilla-beta/rev/bcb00c291057

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/52f1268d8868
https://hg.mozilla.org/releases/mozilla-esr52/rev/d2d15b1d6052
https://hg.mozilla.org/releases/mozilla-esr52/rev/bcb00c291057

Comment 43

[Andrei Vaida [:avaida]]

Setting qe-verify- based on jya's assessment on testing needs (Comment 32) and the fact that this patch has automated coverage.

Comment 45

[Jean-Yves Avenard [:jya]]

It is still happening :(
https://crash-stats.mozilla.com/report/index/b456f391-f739-4712-baea-06e9e2170220

I don't understand what is happening here.
The patches earlier, always check that if Data() returned null, then we must have Size() return 0; everything else is an error (OOM) and will be aborted. 

So it crashes here:
https://hg.mozilla.org/mozilla-central/annotate/c3cbadc5d2fa/dom/media/platforms/agnostic/OpusDecoder.cpp#l189

because aData() returned nullptr.

However, 3 lines above we have:
https://hg.mozilla.org/mozilla-central/annotate/18b5d19d0abc/dom/media/platforms/agnostic/OpusDecoder.cpp#l188

  uint32_t frames_number = opus_packet_get_nb_frames(aSample->Data(),
                                                     aSample->Size());
  if (frames_number <= 0) {

opus_packet_get_nb_frames does:
https://dxr.mozilla.org/mozilla-central/source/media/libopus/src/opus_decoder.c?q=opus_packet_get_nb_frames&redirect_type=direct#944

Which check that Size() isn't null.

So if Size() isn't 0, then Data() can't be nullptr either....

I can of course add an early return ...


Oh noooooooo..
As I'm re-read the code, I see what's going on...

DOH!!!!
unother signed/unsigned issue !

Comment 46

[Jean-Yves Avenard [:jya]]

Attachment: Follow-up, fix invalid unsigned comparison. r?gerald

MozReview-Commit-ID: 26bxAyhxbsx

Comment 47

[Jean-Yves Avenard [:jya]]

Attachment: Follow-up, fix invalid unsigned comparison. r?gerald

MozReview-Commit-ID: 26bxAyhxbsx

Comment 48

[Jean-Yves Avenard [:jya]]

Comment on attachment 8839481 [details] [diff] [review]
Follow-up, fix invalid unsigned comparison. r?gerald

fixed indent

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d0ea0c1c04
Follow-up, fix invalid unsigned comparison. r=gerald

Comment 50

[Jean-Yves Avenard [:jya]]

Comment on attachment 8839482 [details] [diff] [review]
Follow-up, fix invalid unsigned comparison. r?gerald

Approval Request Comment
[Feature/Bug causing the regression]: 1320705
[User impact if declined]: crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: So now, that is the proper fix. Though the previous fixes did fix a problem, so nothing has been lost.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: just that one
[Is the change risky?]: No
[Why is the change risky/not risky?]: We no longer do signed comparison on a unsigned integer !
[String changes made/needed]: none

Comment 51

[Julien Cristau [:jcristau]]

Comment on attachment 8839482 [details] [diff] [review]
Follow-up, fix invalid unsigned comparison. r?gerald

fix crashes due to wrong signed/unsigned comparison, aurora53+, beta52+

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c7d0ea0c1c04

Comment 53

[Ryan VanderMeulen [:RyanVM]]

This last patch needs rebasing for the Aurora uplift.

Comment 54

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/eeeb9c132d0e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6d4e454134d2a383419a49f4daf634771d254d94

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/6d4e454134d2