463756 - Crash [@ vorbis_synthesis]

Status: RESOLVED FIXED

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

Description

[Paul Nickerson]

Attachment: corrupted file

This one gets hit almost immediately when I start fuzzing, so it's blocking my tests. It's not exploitable, just annoying.

  if(mode==-1)return(OV_EBADPACKET);
026C560A  cmp         dword ptr [mode],0FFFFFFFFh 
026C560E  jne         vorbis_synthesis+9Ah (26C561Ah) 
026C5610  mov         eax,0FFFFFF78h 
026C5615  jmp         vorbis_synthesis+1F4h (26C5774h) 
  
  vb->mode=mode;
026C561A  mov         ecx,dword ptr [vb] 
026C561D  mov         edx,dword ptr [mode] 
026C5620  mov         dword ptr [ecx+28h],edx 
  vb->W=ci->mode_param[mode]->blockflag;
026C5623  mov         eax,dword ptr [mode] 
026C5626  mov         ecx,dword ptr [ci] 
026C5629  mov         edx,dword ptr [ecx+eax*4+20h] 
026C562D  mov         eax,dword ptr [vb] 
026C5630  mov         ecx,dword ptr [edx] <-- Access violation reading location 0x00000000.

Comment 1

[Paul Nickerson]

Attachment: open this

Comment 2

[Paul Nickerson]

Attachment: stack

Comment 3

[cajbir (:cajbir)]

Fix from liboggz maintainer:

http://trac.annodex.net/changeset/3773

"oggz_read: return an error when a hole (ie. missing sequence number) is detected in the headers of a track, as such header corruption cannot be tolerated by decoders. Beyond the headers, skip holes in data as before for robustness.

This should fix Mozilla bug 463756: https://bugzilla.mozilla.org/show_bug.cgi?id=463756 which crashed in vorbis_synthesize() after attempting to decode with corrupt headers. The method of this fix is adapted from libvorbisfile, but here should work for any content type. Tested with the file attached to the above bug, with fishsound-info and oggplay-info, both of which previously crashed. "

I'll apply to FF and test.

Comment 4

[Paul Nickerson]

Did the new code work? I tried it and still crash.

Comment 5

[cajbir (:cajbir)]

when you  say you tried it, do you mean you did a build with this change and it still crashed?

It's the weekend here and i'm away so don't have the ability to build and test until monday.

Comment 6

[Paul Nickerson]

Sorry for not getting back to you sooner. Yes, I built with the change and it didn't make any difference.

Comment 7

[Conrad Parker]

liboggz changeset:3773 changes the behaviour of oggz_read() such that it will return OGGZ_ERR_HOLE_IN_DATA when a hole is found in the stream headers. The next step for this bug would be to verify that that error is being reported to liboggplay, and then deal with appropriately in firefox -- ie. by not attempting to decode the rest of the stream.

Comment 8

[Damon Sicore (:damons)]

Assigning to Chris Double.  Chris, if this shouldn't be yours, please let me know.  Just want to make sure people are on the remaining 1.9.1 blockers.

Comment 9

[Paul Nickerson]

I hate to rush anyone, but I can't fuzz Vorbis at all until this is fixed.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chris D, can we update liboggz to get Conrad's fix and do whatever else is needed to handle the error?

Comment 11

[cajbir (:cajbir)]

We've already updated liboggz and have Conrad's fix. Now it needs some investigation to see if liboggplay is handling the error, passing it to us, and if we are handling that. I saw Conrad asking Viktor about it in irc today so he may be able to shed some light on what liboggplay is doing.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't actually crash in Paul's testcase on trunk. Paul, how about you?

Comment 13

[Paul Nickerson]

It seems that you're right. I tested a few nightlies from the past couple days and it looks like it was patched on March 4th. Thanks to whoever did that. Can I mark this as resolved?

Comment 14

[Martijn Wargers (dead)]

If you don't see the crash anymore, you can close the bug.

Comment 15

[Paul Nickerson]

Attachment: another corrupted file

I'm reopening this bug because it's not yet fixed. Attached is a video file that shows the same bug.

Comment 16

[Paul Nickerson]

Attachment: video tag for attached variation - open this

Comment 17

[Paul Nickerson]

Attachment: new stack - slightly different

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Paul, I think since we fixed a real bug here you should file a new bug for the new testcase.

Comment 19

[Paul Nickerson]

Created bug 481933 for the variation