1604331 - null pointer passed as argument 2, which is declared to never be null in dom/media/gtest/mp4_demuxer/TestMP4.cpp

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Found with m-c 20191213-5343dd9f67f3
This is triggered with an UBSan build while running gtests. To enable this check add the following to your mozconfig:

ac_add_options --enable-undefined-sanitizer="nonnull-attribute"

[ RUN      ] rust.MP4MetadataEmpty
src/dom/media/gtest/mp4_demuxer/TestMP4.cpp:49:18: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here
    #0 0x7f4abd2179f2 in vector_reader(unsigned char*, unsigned long, void*) src/dom/media/gtest/mp4_demuxer/TestMP4.cpp:49:3
    #1 0x7f4acd456df8 in _$LT$mp4parse_capi..Mp4parseIo$u20$as$u20$std..io..Read$GT$::read::h4273be593bc973a5 src/media/mp4parse-rust/mp4parse_capi/src/lib.rs:331:17
    #2 0x7f4acd45ad21 in std::io::Read::read_exact::h5b5013b63263c7aa /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/io/mod.rs:768:18
    #3 0x7f4acd4844ee in byteorder::io::ReadBytesExt::read_u32::h14bcd62332fa44c1 src/third_party/rust/byteorder/src/io.rs:217:13
    #4 0x7f4acd472260 in mp4parse::be_u32::h98dcb131c2dafa2d src/media/mp4parse-rust/mp4parse/src/lib.rs:2687:4
    #5 0x7f4acd467a1d in mp4parse::read_box_header::h5d13aebc84a81716 src/media/mp4parse-rust/mp4parse/src/lib.rs:780:17
    #6 0x7f4acd46a488 in mp4parse::BoxIter$LT$T$GT$::next_box::h437f5d3d96b17422 src/media/mp4parse-rust/mp4parse/src/lib.rs:732:16
    #7 0x7f4acd4732b5 in mp4parse::read_mp4::h0b919823d2d76b10 src/media/mp4parse-rust/mp4parse/src/lib.rs:869:28
    #8 0x7f4acd45714e in mp4parse_read src/media/mp4parse-rust/mp4parse_capi/src/lib.rs:383:12
    #9 0x7f4abd216e8c in rust_MP4MetadataEmpty_Test::TestBody() src/dom/media/gtest/mp4_demuxer/TestMP4.cpp:90:8
    #10 0x7f4abca5299f in testing::Test::Run() src/testing/gtest/gtest/src/gtest.cc:2519:5
    #11 0x7f4abca539e6 in testing::TestInfo::Run() src/testing/gtest/gtest/src/gtest.cc:2695:11
    #12 0x7f4abca5433a in testing::TestCase::Run() src/testing/gtest/gtest/src/gtest.cc:2813:28
    #13 0x7f4abca6270b in testing::internal::UnitTestImpl::RunAllTests() src/testing/gtest/gtest/src/gtest.cc:5179:43
    #14 0x7f4abca62164 in testing::UnitTest::Run() src/testing/gtest/gtest/src/gtest.cc:4788:10
    #15 0x7f4abcaa3f94 in mozilla::RunGTestFunc(int*, char**) src/testing/gtest/mozilla/GTestRunner.cpp:158:10
    #16 0x7f4ac9e987ee in XREMain::XRE_mainStartup(bool*) src/toolkit/xre/nsAppRunner.cpp:3764:16
    #17 0x7f4ac9ea1aab in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4722:12
    #18 0x7f4ac9ea271b in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4818:21
    #19 0x55a694d7adf2 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:217:22
    #20 0x55a694d7a500 in main src/browser/app/nsBrowserApp.cpp:339:16

Comment 1

[Michael Froman [:mjf]]

Marking P2 for now. John, do you know who should take a look at this?

Comment 2

[John Lin [:jhlin][:jolin]]

It looks like the test creates an empty vector as data source on purpose at [1] but the reader function [2] doesn't handle it properly. It should check the buffer validness before sending the pointer to memcpy. I'll upload a patch for it.

[1] https://searchfox.org/mozilla-central/source/dom/media/gtest/mp4_demuxer/TestMP4.cpp#86
[2] https://searchfox.org/mozilla-central/source/dom/media/gtest/mp4_demuxer/TestMP4.cpp#38

Comment 3

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1604331 - read source buffer only when necessary. r?jbauman

When receiving an empty source buffer, vector_reader() passes a null pointer to
memcpy() and causes undefined behavior. Add a check to avoid it.

Comment 4

[Tyson Smith [:tsmith]]

John, is there someone else that can do the review? I don't think rillian is available?

Comment 5

[John Lin [:jhlin][:jolin]]

(In reply to Tyson Smith [:tsmith] from comment #4)

John, is there someone else that can do the review? I don't think rillian is available?

:jbauman is in charge of mp4parse now. I'll change the reviewer on Phabricator.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3e168f314be
read source buffer only when necessary. r=jbauman

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset f3e168f314be (bug 1604331) for rust.MP4Metadata failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=VCk03P8aTY2-4azDQlniZA.0&fromchange=1b228ca5d679f60bc5f40f646e0515dd84670863&searchStr=gtest&tochange=0b1d5c36d85f4b7a8063f5d9412723f6b14234ab

Backout link: https://hg.mozilla.org/integration/autoland/rev/0b1d5c36d85f4b7a8063f5d9412723f6b14234ab

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313511099&repo=autoland&lineNumber=5843

[task 2020-08-20T01:57:04.040Z] 01:57:04     INFO -  TEST-START | rust.MP4MetadataEmpty
[task 2020-08-20T01:57:04.040Z] 01:57:04  WARNING -  TEST-UNEXPECTED-FAIL | rust.MP4MetadataEmpty | Expected equality of these values:
[task 2020-08-20T01:57:04.040Z] 01:57:04     INFO -    rv
[task 2020-08-20T01:57:04.040Z] 01:57:04     INFO -      Which is: 2
[task 2020-08-20T01:57:04.040Z] 01:57:04     INFO -    MP4PARSE_STATUS_UNSUPPORTED
[task 2020-08-20T01:57:04.040Z] 01:57:04     INFO -      Which is: 3 @ /builds/worker/checkouts/gecko/dom/media/gtest/mp4_demuxer/TestMP4.cpp:106
[task 2020-08-20T01:57:04.040Z] 01:57:04  WARNING -  TEST-UNEXPECTED-FAIL | rust.MP4MetadataEmpty | test completed (time: 1ms)
[task 2020-08-20T01:57:04.040Z] 01:57:04     INFO -  TEST-START | rust.MP4Metadata
[task 2020-08-20T01:57:04.041Z] 01:57:04  WARNING -  TEST-UNEXPECTED-FAIL | rust.MP4Metadata | Expected: (nullptr) != (parser), actual: (nullptr) vs NULL @ /builds/worker/checkouts/gecko/dom/media/gtest/mp4_demuxer/TestMP4.cpp:124
[task 2020-08-20T01:57:04.041Z] 01:57:04  WARNING -  TEST-UNEXPECTED-FAIL | rust.MP4Metadata | test completed (time: 2ms)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4412df5d4011
read source buffer only when necessary. r=jbauman

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4412df5d4011