Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Description • 8 years ago

Spawned from bug 1301065 comment 10:
The track's duration in MP4MetadataStagefright is reported as 107841296855264, which is the result of an overflow.  The calculation at https://dxr.mozilla.org/mozilla-central/rev/ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#1095 is the same as the Rust version, minus the implicit overflow checks.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 1 • 8 years ago

P1: Catch duration overflows in mdhd and mehd.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 2 • 8 years ago

P2: gtest with test case from bug 1301065.

We will really need to land bug 1301065 before this bug here, because this gtest will crash unless the rust parser is fixed (I had to disable rust in order to run this test locally).

	Daniel Veditz [:dveditz]
	Comment 3 • 8 years ago

What bad things happen if this overflows? If it's the track's duration do we just play less of it? Or do we actually use this to allocate a too-small buffer somewhere and then try to copy the whole thing into it?

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 4 • 8 years ago

Good questions!
I originally copied the security flags from parent bug 1301065.
But as I said in bug 1301065 comment 12, I don't believe this is a security issue: The duration doesn't have any direct link with memory buffers, it's just used to guide playback timings.
It could be related to a key type in a map: The map is the sequence of media frame, each with their own key=time; and the duration could be used to compare with frame times, so in the worst case an absurdly-high duration will not match any frame.

And I now see that bug 1301065 was re-opened, so I'll just do the same here (If I can find out how to do it).

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 5 • 8 years ago

Looks like I'm not allowed to remove the "Security-Sensitive Core Bug" flag (that I set myself!)
Daniel, would you mind clearing it, or teaching me how to? Thank you.

	Matthew Gregan [:kinetik]
	Comment 6 • 8 years ago

Comment on attachment 8789363 [details] [diff] [review]
1301293-p1-duration-overflows-mdhd-mehd.patch

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +1091,5 @@
>              if (!mLastTrack->timescale) {
>                  return ERROR_MALFORMED;
>              }
> +            if (duration > INT64_MAX / 1000000) {
> +                return ERROR_MALFORMED;

I think we can do better than this if we take a similar approach to Chromium's TimeDeltaFromRational, e.g. https://crbug.com/345888 and the related change to track_run_iterator.cc.

I'm going to r- the Rust changes for the same reason.

	Matthew Gregan [:kinetik]
	Comment 7 • 8 years ago

Comment on attachment 8789364 [details] [diff] [review]
1301293-p2-gtest-case-1301065.patch

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

With the changes suggested to the duration handling, I think this testcase turns into an mHeader == true case.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 8 • 8 years ago

(In reply to Matthew Gregan [:kinetik] from comment #6)
> 1301293-p1-duration-overflows-mdhd-mehd.patch
> > +            if (duration > INT64_MAX / 1000000) {
> I think we can do better than this if we take a similar approach to
> Chromium's TimeDeltaFromRational, e.g. https://crbug.com/345888 and the
> related change to track_run_iterator.cc.

Yes, I could use something like SaferMultDiv from VideoUtils -- function that I originally suggested to jya, not knowing about the Chromium solution.

I did think about something like that, but then:
- Are we reasonably expected to deal with durations of more than the 9e18 units that the current solution supports? (That's more than 6 years at 44kHz)
- Stagefright is supposed to die; We're in minimal-maintenance mode, so is it worth spending time on throw-away code that has doubtful benefits?


(In reply to Matthew Gregan [:kinetik] from comment #7)
> 1301293-p2-gtest-case-1301065.patch
> With the changes suggested to the duration handling, I think this testcase
> turns into an mHeader == true case.

Do you mean you think it's a useless test case?
Even though the result is indeed indistinguishable from other possible parsing errors, I think it has the merit of exercising the code: The test currently crashes because of the Rust bug, so it would prove that the Rust fix in bug 1301065 prevents such crashes. Also I have manually verified that the test case triggers my new code path.
What do you propose we should do instead?

	Matthew Gregan [:kinetik]
	Comment 9 • 8 years ago

(In reply to Gerald Squelart [:gerald] from comment #8)
> Yes, I could use something like SaferMultDiv from VideoUtils -- function
> that I originally suggested to jya, not knowing about the Chromium solution.
> 
> I did think about something like that, but then:
> - Are we reasonably expected to deal with durations of more than the 9e18
> units that the current solution supports? (That's more than 6 years at 44kHz)
> - Stagefright is supposed to die; We're in minimal-maintenance mode, so is
> it worth spending time on throw-away code that has doubtful benefits?

Isn't the policy not to reject files that Chrome accepts?

> Do you mean you think it's a useless test case?
> Even though the result is indeed indistinguishable from other possible
> parsing errors, I think it has the merit of exercising the code: The test
> currently crashes because of the Rust bug, so it would prove that the Rust
> fix in bug 1301065 prevents such crashes. Also I have manually verified that
> the test case triggers my new code path.
> What do you propose we should do instead?

What I was trying to say here is: if the duration handling is changed to work as I suggested rather than reject the file, the test also needs to be updated to expect that the file parses correctly rather than being treated as invalid.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 10 • 8 years ago

A better timescaled units to microseconds routine lessens the likelihood of
overflows.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 11 • 8 years ago

gtest with test case from bug 1301065

Altered copies of the base test case helps verify different duration overflow
boundaries.

Track durations are now checked (including for previous test cases).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c900357e2047ad5184c73cbe568ac0a4dadac3

	Matthew Gregan [:kinetik]
	Comment 12 • 8 years ago

Comment on attachment 8790707 [details] [diff] [review]
1301293-p2-gtest-case-1301065.patch v2

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

::: media/libstagefright/gtest/TestParser.cpp
@@ +156,5 @@
>  struct TestFileData
>  {
>    const char* mFilename;
>    uint32_t mNumberVideoTracks;
> +  int64_t mVideoDuration;

Might pay to clarify somewhere (here or where the tests are listed) that this is the duration of the first (video/audio) track.  Same comment applies for the audio duration.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 13 • 8 years ago

(In reply to Matthew Gregan [:kinetik] from comment #12)
> Comment on attachment 8790707 [details] [diff] [review]
> 1301293-p2-gtest-case-1301065.patch v2
> 
> ::: media/libstagefright/gtest/TestParser.cpp
> @@ +160,1 @@
> > +  int64_t mVideoDuration;
> 
> Might pay to clarify somewhere (here or where the tests are listed) that
> this is the duration of the first (video/audio) track.  Same comment applies
> for the audio duration.

Makes sense, will do.

Thank you for the reviews, and pushing for support of extreme cases.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 14 • 8 years ago

Addressed review comments in comment 12. Carrying r+.

	Pulsebot
	Comment 15 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94e42d7c9bf
Catch duration overflows in mdhd and mehd. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ee1b3c2df13
Create gtest for test case from bug 1301065. r=kinetik

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c94e42d7c9bf
https://hg.mozilla.org/mozilla-central/rev/1ee1b3c2df13