526097 - mochitest-plain: intermittent crash in [@ auto_rcalc_vorbis]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The call stacks are different in these 2 logs, but they're both access violations triggered at the same point, so I'm assuming they're related.


http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1257211645.1257212196.11574.gz&fulltext=1
WINNT 5.2 mozilla-1.9.2 test mochitests on 2009/11/02 17:27:25  

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1257128806.1257129548.16352.gz&fulltext=1
WINNT 5.2 mozilla-1.9.2 test mochitests on 2009/11/01 18:26:46

Comment 1

[Matthew Gregan [:kinetik]]

The first one is 0  xul.dll!auto_rcalc_vorbis [oggz_auto.c:404846c8ace1 : 954 + 0x8], which looks new to me.

The second one is a crash in oggplay_buffer_set_last_data, which is probably bug 515217 (fixed on trunk, still waiting for 1.9.2 approval).

Comment 2

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264083500.1264086504.27093.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/01/21 06:18:20
s: win32-slave12

Comment 3

[Shawn Wilsher :sdwilsh]

WINNT 5.2 mozilla-central debug test mochitests-1/5 [testfailed] Started 07:59, finished 08:36
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267027153.1267029313.29671.gz&fulltext=1

Comment 4

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1267577094.1267579213.27374.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/03/02 16:44:54
s: win32-slave39

Comment 5

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

This shows up running valgrind on content/media/test/test_playback.html:

1. ==29624== Invalid read of size 4
2. ==29624==    at 0x5BD4825: auto_rcalc_vorbis (oggz_auto.c:954)
3. ==29624==    by 0x5BD7B85: oggz_read_update_gp (oggz_read.c:278)
4. ==29624==    by 0x5BD7172: oggz_dlist_reverse_iter (oggz_dlist.c:166)
5. ==29624==    by 0x5BD8269: oggz_read_sync (oggz_read.c:469)
6. ==29624==    by 0x5BD85AF: oggz_read (oggz_read.c:612)
7. ==29624==    by 0x5BC7738: oggplay_initialise (oggplay.c:126)
8. ==29624==    by 0x5C00679: nsOggDecodeStateMachine::LoadOggHeaders(nsChannelReader*) (nsOggDecoder.cpp:1795)
9. ==29624==    by 0x5C01E67: nsOggDecodeStateMachine::Run() (nsOggDecoder.cpp:1430)
10. ==29624==    by 0x60F4C6F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527)
11. ==29624==    by 0x60BABD8: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
12. ==29624==    by 0x60F523C: nsThread::ThreadFunc(void*) (nsThread.cpp:254)
13. ==29624==    by 0x7439DC2: _pt_root (ptthread.c:228)
14. ==29624==  Address 0x1c5cdc90 is 4 bytes after a block of size 44 alloc'd
15. ==29624==    at 0x4C25528: malloc (vg_replace_malloc.c:236)
16. ==29624==    by 0x5BD53C6: auto_calc_vorbis (oggz_auto.c:704)
17. ==29624==    by 0x5BD7F90: oggz_read_sync (oggz_read.c:421)
18. ==29624==    by 0x5BD85AF: oggz_read (oggz_read.c:612)
19. ==29624==    by 0x5BC7738: oggplay_initialise (oggplay.c:126)
20. ==29624==    by 0x5C00679: nsOggDecodeStateMachine::LoadOggHeaders(nsChannelReader*) (nsOggDecoder.cpp:1795)
21. ==29624==    by 0x5C01E67: nsOggDecodeStateMachine::Run() (nsOggDecoder.cpp:1430)
22. ==29624==    by 0x60F4C6F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527)
23. ==29624==    by 0x60BABD8: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
24. ==29624==    by 0x60F523C: nsThread::ThreadFunc(void*) (nsThread.cpp:254)
25. ==29624==    by 0x7439DC2: _pt_root (ptthread.c:228)
26. ==29624==    by 0x4E31A03: start_thread (pthread_create.c:300)

Comment 6

[Matthew Gregan [:kinetik]]

This happens with bug498855-1.ogv when dealing with packetno 3 (110 bytes).  This is a fuzzer generated file, so it's probably an invalid packet.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: make error fatal for easier debugging

Comment 8

[Matthew Gregan [:kinetik]]

Things go wrong even earlier.  When dealing with packetno 2 (802 bytes), auto_calc_vorbis calculates size_realloc_bytes as smaller than sizeof(auto_calc_vorbis_info_t) (size = -2), so bails early and does not complete setup.  It returns an error of -1, which the caller (oggz_auto_calculate_granulepos -> oggz_read_sync) treats as an unknown granulepos rather than an error.

vorbose reports:
WARN header: Expecting a Vorbis stream header, got
             some other packet type instead. Stream is not
             decodable as Vorbis I.

WARN stream: page did not contain a valid Vorbis I identification
             header. Stream is not decodable as Vorbis I.

WARN codebk: Premature EOP while parsing codebook.
WARN codebk: Invalid codebook; stream is undecodable.
WARN stream: next packet is not a valid Vorbis I setup header as expected.
             Stream is not decodable as Vorbis I.

WARN stream: 363 bytes of garbage before page 6

No logical Vorbis streams found in data.

Comment 9

[Matthew Gregan [:kinetik]]

As an ignorant bystander, it seems like oggz's approach to finding the mode data at the end of the packet is incorrect because there's no way for it to detect that the packet may be truncated:

http://mxr.mozilla.org/mozilla-central/source/media/liboggz/src/liboggz/oggz_auto.c#726

Conrad, any ideas?

Comment 10

[Benjamin Smedberg]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268068425.1268070905.20769.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/03/08 09:13:45

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: patch v0

This adds a length check to mode_sizes so metrics calculation does not read out of the array bounds when it's uninitialized.  When trying to calculate metrics in the case where mode_sizes is uninitialized the code returns a 0 granulepos, which effectively marks the stream as invalid (by acting as if every page is a header).

There's also a huge chunk of dead code deleted from auto_calc_vorbis so that any later debugging/auditing in this area is easier.

This fix isn't suitable for upstream because it's just acting as a bandaid to ensure we don't read out of array bounds.  The correct fix is probably to modify auto_calc_vorbis to read the Vorbis headers in a forward direction according to the spec, rather than trying to grope backwards to find the one piece it's interested in.  I looked at doing this, but it involves dragging in a huge amount of Vorbis initialization code, and honestly I think the entire approach of including this code in liboggz rather than using the code in libvorbis is utterly mad.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a summary of why this should be blocking? Do we think it's a crash that's affecting a lot of users or a potential security vulnerability?

We'd probably look at a baked and nominated patch, of course.

Comment 13

[Matthew Gregan [:kinetik]]

Sorry, I was a bit overeager with flags.  It shouldn't block, but I think it'll be worth taking on branch once it has baked on trunk for a bit.  I'll nominate it when it has had some time to bake.

Comment 14

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/f072b740b5eb

Comment 15

[Matthew Gregan [:kinetik]]

That burnt the tree on Windows due to C89 declaration fussiness.  Bustage fixed pushed (including updated .patch file): http://hg.mozilla.org/mozilla-central/rev/99a292b9df0a