787805 - Intermittent 752784-1.html | crash @ FileMediaResource::EnsureLengthInitialized

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Because FileMediaResource::EnsureLengthInitialized() dereferences mInput without null-checking it:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14906033&tree=Mozilla-Inbound

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 657723 [details] [diff] [review]
Patch

Review of attachment 657723 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaResource.cpp
@@ +1030,5 @@
>  
>  void FileMediaResource::EnsureLengthInitialized()
>  {
>    mLock.AssertCurrentThreadOwns();
> +  if (mSizeInitialized || !mInput) {

But this means EnsureLengthInitialized does not, in fact, ensure the length is initialized.

Seems to me the caller shouldn't be getting to call EnsureLengthInitialized if mInput is null.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Usually we null check mInput and bail out from the methods that need to call EnsureLengthInitialized(), but we don't in at least two places: FileMediaResource::GetCachedRanges() and FileMediaResource::GetLength(), so if we remove the mInput null check from EnsureLengthInitialized() it needs to be duplicated in at least those two places, which leaks the abstraction EnsureLengthInitialized creates.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e28fd5ccdb40

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, I think I meant to r- that. Sorry.

(In reply to Chris Pearce (:cpearce) from comment #3)
> Usually we null check mInput and bail out from the methods that need to call
> EnsureLengthInitialized(), but we don't in at least two places:
> FileMediaResource::GetCachedRanges() and FileMediaResource::GetLength(), so
> if we remove the mInput null check from EnsureLengthInitialized() it needs
> to be duplicated in at least those two places, which leaks the abstraction
> EnsureLengthInitialized creates.

I don't understand what you mean by "leaks the abstraction". mInput being non-null should be a precondition for calling EnsureLengthInitialized. So either GetCachedRanges should also have a precondition of mInput being non-null, or it should return no ranges when mInput is null. Similar for GetLength.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2

Let's try that again, but enforcing that mInput is non null in the callers. I think we should be taking the lock before reading mSize in GetCachedDataEnd and GetNextCachedData too.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 657769 [details] [diff] [review]
Patch 2

Review of attachment 657769 [details] [diff] [review]:
-----------------------------------------------------------------

Why are we calling these methods when mInput is null, anyway?

::: content/media/MediaResource.cpp
@@ +959,5 @@
>    }
>    virtual int64_t GetLength() {
>      MutexAutoLock lock(mLock);
> +    if (!mInput) {
> +      return -1;

Wouldn't zero be safer here?

@@ +976,5 @@
>    }
> +  virtual int64_t GetCachedDataEnd(int64_t aOffset) {
> +    MutexAutoLock lock(mLock);
> +    if (!mInput) {
> +      return -1;

Wouldn't aOffset be a safer return value here?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 657769 [details] [diff] [review]
> Patch 2
> 
> Review of attachment 657769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are we calling these methods when mInput is null, anyway?

Because mInput is annulled when the decoder is being shutdown, and that can happen while we're in the middle of seeking or calculating buffered ranges (where we call these methods).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Because mInput is annulled when the decoder is being shutdown, and that can
> happen while we're in the middle of seeking or calculating buffered ranges
> (where we call these methods).

Aha! Please document that where mInput is declared.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 657769 [details] [diff] [review]
Patch 2

Review of attachment 657769 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that.

::: content/media/MediaResource.cpp
@@ +959,5 @@
>    }
>    virtual int64_t GetLength() {
>      MutexAutoLock lock(mLock);
> +    if (!mInput) {
> +      return -1;

Actually I think it would be best to return mSize if we've got it, otherwise return 0.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 657936 [details] [diff] [review]
Patch 3

Review of attachment 657936 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that fixed.

::: content/media/MediaResource.cpp
@@ +967,5 @@
>    virtual int64_t GetNextCachedData(int64_t aOffset)
>    {
> +    MutexAutoLock lock(mLock);
> +    if (!mInput) {
> +      return aOffset;

Should return -1. It's not correct to say that aOffset is the start of a cached data block.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/482edf1460dc

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/482edf1460dc