1286272 - int32_t read_file(const char*): Assertion `rv2 == MP4PARSE_OK' failed

Status: RESOLVED WONTFIX

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

Description

[Ucha Gobejishvili]

Attachment: testcase-751.mp4

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606113944

Steps to reproduce:

Hello guys, just tried to fuzz new product rust mp4 parser, after couple of second i got this crash. 

Build instruction copy from here: https://travis-ci.org/mozilla/mp4parse-rust/jobs/141199847
Run the testcase.


Actual results:

test: test.cc:239: int32_t read_file(const char*): Assertion `rv2 == MP4PARSE_OK' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff6b79418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff6b79418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff6b7b01a in __GI_abort () at abort.c:89
#2  0x00007ffff6b71bd7 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x4d3dbf "rv2 == MP4PARSE_OK", file=file@entry=0x4d3ba4 "test.cc", line=line@entry=239, 
    function=function@entry=0x4d3f90 <read_file(char const*)::__PRETTY_FUNCTION__> "int32_t read_file(const char*)") at assert.c:92
#3  0x00007ffff6b71c82 in __GI___assert_fail (assertion=assertion@entry=0x4d3dbf "rv2 == MP4PARSE_OK", file=file@entry=0x4d3ba4 "test.cc", line=line@entry=239, 
    function=function@entry=0x4d3f90 <read_file(char const*)::__PRETTY_FUNCTION__> "int32_t read_file(const char*)") at assert.c:101
#4  0x0000000000408661 in read_file (filename=<optimized out>) at test.cc:239
#5  0x0000000000404e0e in main (argc=<optimized out>, argv=0x7fffffffdc08) at test.cc:264
(gdb) i r
rax            0x0      0
rbx            0x7ffff7ff5000   140737354092544
rcx            0x7ffff6b79418   140737332614168
rdx            0x6      6
rsi            0x3ad4   15060
rdi            0x3ad4   15060
rbp            0x4d3dbf 0x4d3dbf
rsp            0x7fffffffd888   0x7fffffffd888
r8             0x7524d0 7677136
r9             0xffffffffff000000       -16777216
r10            0x8      8
r11            0x206    518
r12            0xef     239
r13            0x4d3f90 5062544
r14            0x7fffffffdc10   140737488346128
r15            0x0      0
rip            0x7ffff6b79418   0x7ffff6b79418 <__GI_raise+56>
eflags         0x206    [ PF IF ]
cs             0x33     51
ss             0x2b     43
ds             0x0      0
es             0x0      0
fs             0x0      0
gs             0x0      0

Comment 1

[:Gijs (he/him)]

:rillian, I'm told you might know more about this and where it should live?

Comment 2

[Ralph Giles (:rillian)]

Thanks for fuzzing. Just to confirm, this is test.cc from the upstream github repo, not code we have in firefox?

I guess for fuzzing we should convert all those asserts to error returns?

Comment 3

[Ralph Giles (:rillian)]

Attachment: Convert asserts to error returns

I can't reproduce the assert with the testcase, I just get an parse failure with unexpected EOF.

Does the attached patch let the fuzzer run further?

Comment 4

[Ucha Gobejishvili]

(In reply to Ralph Giles (:rillian) needinfo me from comment #2)
> Thanks for fuzzing. Just to confirm, this is test.cc from the upstream
> github repo, not code we have in firefox?
> 
> I guess for fuzzing we should convert all those asserts to error returns?

Yes it's just test.cc, was easier for me to fuzz.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8770209 [details] [diff] [review]
Convert asserts to error returns

I guess converting the asserts is fine but please make it fatally exit on parse failure.  This is for testing/development, it's not a fuzzing harness and I don't want to support it was one.  We already have examples/afl-capi.rs and examples/afl.rs for this purpose.

Comment 6

[Ralph Giles (:rillian)]

Ok, I'll not land the patch upstream. As Matthew pointed out, there's nothing afl-specific about the afl harnesses, so those are a better choice to use. You may want to compile with `cargo test --features=abort_on_panic` to get a crash closer to failing code, depending on the fuzzing technology you're using.

This bug can be made public public since there is no security issue and no affect on users.

Comment 7

[Ucha Gobejishvili]

(In reply to Ralph Giles (:rillian) needinfo me from comment #6)
> Ok, I'll not land the patch upstream. As Matthew pointed out, there's
> nothing afl-specific about the afl harnesses, so those are a better choice
> to use. You may want to compile with `cargo test --features=abort_on_panic`
> to get a crash closer to failing code, depending on the fuzzing technology
> you're using.
> 
> This bug can be made public public since there is no security issue and no
> affect on users.

Agree

Comment 8

[Ralph Giles (:rillian)]

Ok, closing. We believe the assert is working as intended for quick-failure testing. Please use either the other example code intended for fuzzing (comment #5), or the attached patch for further work.

Thanks for the report!