1187067 - Stagefright: NULL deref [@stagefright::MPEG4Extractor::parseChunk]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

This may be related to Bug 1156517 but the call stacks do differ.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.mp4

Comment 2

[angregorio32]

[Tracking Requested - why for this release]:

Comment 3

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

Attachment: 1187067-gtests-with-test_case_mp4.patch

Added gtests using the bug's test file, running simple checks and many parsing runs to catch parser crashes.
(I know it's a bit ugly to include the binary file directly into the test, but it makes things much easier than trying to read a separate file at runtime.)

Running this gtest alone (without the upcoming fix) results in crashes similar to what is found here and in other stagefright bugs.

It shows that mLastTrack is dereferenced when null, which would explain this bug's particular crash, as it happens when calling mLastTrack->sampleTable->some_method(); mLastTrack being null, sampleTable (a smart pointer) could point anywhere, and this is caught when running its operator->().

Comment 4

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

Attachment: 1187067-nullcheck-mLastTrack.patch

Null-check mLastTrack before dereferencing it. With this, the new gtests in the other patch run through without crashing anymore.

The issue was that mLastTrack was first set when encountering a "trak" box.
But most other boxes assumed that a track existed and just used mLastTrack without checking.

This is fixed by checking mLastTrack and returning ERROR_MALFORMED when null (with a couple of special cases, see patch).

I believe this patch will probably fix a few other bugs with slightly different signatures, I'll try and mark them soon.

Comment 5

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

The gtest patch requires the patch in bug 1201404.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8656431 [details] [diff] [review]
1187067-gtests-with-test_case_mp4.patch

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

Better than nothing. I assume the test data is short enough the partial-data parse scan doesn't take appreciable time?

::: media/libstagefright/gtest/TestParser.cpp
@@ +300,5 @@
> +  0x73, 0x74, 0x00, 0x00, 0x00, 0x25, 0xa9, 0x74, 0x6f, 0x6f, 0x00, 0x00,
> +  0x00, 0x1d, 0x64, 0x61, 0x74, 0x61, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
> +  0x00, 0x00, 0x4c, 0x61, 0x76, 0x66, 0x35, 0x36, 0x2e, 0x33, 0x33, 0x2e,
> +  0x31, 0x30, 0x31
> +};

I have no objection to small test data inline like this, but fwiw it's no trouble to load external files either. Just add the filename to TEST_HARNESS_FILES.gtest in moz.build and they'll be available in the gtest environment.

Nice pretty-printing, btw.

@@ +301,5 @@
> +  0x00, 0x1d, 0x64, 0x61, 0x74, 0x61, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
> +  0x00, 0x00, 0x4c, 0x61, 0x76, 0x66, 0x35, 0x36, 0x2e, 0x33, 0x33, 0x2e,
> +  0x31, 0x30, 0x31
> +};
> +static const size_t test_case_mp4_len = sizeof (test_case_mp4) / sizeof (test_case_mp4[0]);

Use ArrayLength(test_case_mp4) here instead.

@@ +378,5 @@
> +}
> +
> +TEST(stagefright_MoofParser, test_case_mp4_skimming)
> +{
> +  static const size_t step = 4u;

This can just be const, no?

@@ +382,5 @@
> +  static const size_t step = 4u;
> +  nsRefPtr<MediaByteBuffer> buffer = new MediaByteBuffer(test_case_mp4_len);
> +  buffer->AppendElements(test_case_mp4, test_case_mp4_len);
> +  mozilla::Monitor monitor("MP4Metadata::HasCompleteMetadata");
> +  mozilla::MonitorAutoLock mon(monitor);

You're in a 'using namespace mozilla' here, so the prefix isn't necessary. I don't think mp4_demuxer has a monitor you'd need to distinguish. But up to you.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8656436 [details] [diff] [review]
1187067-nullcheck-mLastTrack.patch

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

I hesitate to ask whether all callers check the return value.

Comment 8

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

Attachment: 1187067-gtests-with-test_case_mp4.patch v2

Update with suggestions from comment 6, carrying r+.

Comment 9

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

(In reply to Ralph Giles (:rillian) from comment #7)
> Comment on attachment 8656436 [details] [diff] [review]
> 1187067-nullcheck-mLastTrack.patch
> 
> Review of attachment 8656436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I hesitate to ask whether all callers check the return value.

And still you did...

With good cause: Some return values are not checked, though in some cases it seems to be on purpose. So I hesitate to just change these here in a hurry, I will open a new bug instead.

Comment 10

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

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

Comment 11

[Carsten Book [:Tomcat]]

gerald are the test failures like the m-other and crashtests in the try run related to this 2 patches here ?

Comment 12

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

(In reply to Carsten Book [:Tomcat] from comment #11)
> gerald are the test failures like the m-other and crashtests in the try run
> related to this 2 patches here ?

No they are not: Each of them is of the "no tests to run" variety, due to a bad combination of test types and paths in the 'try' command; The first build error on XP debug looks like an issue while installing the build/test env on the test machine.

For peace of mind: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f2e74df1d5

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a741f94fc2ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b70b41b300

Comment 15

[Guilherme Lima]

This landed a few days ago, but didn't got marked as fixed.

https://hg.mozilla.org/mozilla-central/rev/a741f94fc2ad
https://hg.mozilla.org/mozilla-central/rev/f5b70b41b300

Comment 16

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

(In reply to Guilherme Lima from comment #15)
> This landed a few days ago, but didn't got marked as fixed.
> 
> https://hg.mozilla.org/mozilla-central/rev/a741f94fc2ad
> https://hg.mozilla.org/mozilla-central/rev/f5b70b41b300

Yeah, something strange happened when it was merged to m-c. I've opened bug 1203300 about it, and was trying to leave this bug here alone not to disturb the evidence. But it should be long enough now, so thank you for resolving this bug.

Comment 17

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

Attachment: 1187067-nullcheck-mLastTrack-beta42.patch

Approval Request Comment
[Feature/regressing bug #]: Media playback, and needed to uplift bug 1156505
[User impact if declined]: Crashes with invalid mp4 files
[Describe test coverage new/current, TreeHerder]: gtest, media mochitests
[Risks and why]: None, only adding null-ptr checks
[String/UUID change made/needed]: n/a

(Simple rebase of main patch, carrying r+)

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8677292 [details] [diff] [review]
1187067-nullcheck-mLastTrack-beta42.patch

Needed for another patch. Taking it, should be in 42 beta 9.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1721db97f72e