512328 - (CVE-2009-3378) Update liboggplay

Status: RESOLVED FIXED

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

Description

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

We should update liboggplay to pickup recent fixes.

Comment 1

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

Attachment: Patch + fixes

* Update liboggplay to rev 8640eb3fddc43ad4.

I had to make some changes to content/media, media/libfishsound and liboggplay's oggplay_seek_cleanup() so that all the mochitests pass after the update. I decided to put them in one patch, as they must all be checked in together else tests fail.
* Fixed our decode logic so that we don't call oggplay_step_decoding() after it's returned E_OGGPLAY_OK (it does this when it reaches EOF. This prevents infinite loops in oggplay_step_decoding).
* Changed nsOggDecodeStateMachine::LoadOggHeaders() so that if oggplay_get_video_y_size fails, we set the video size to 0x0. It can fail when the theora stream has been corrupted.
* Some files which didn't play now play, so changed manifest.js to reflect that.
* liboggplay_seek_cleanup now calls fish_sound_reset() when it seeks to 0. This resets the vorbis packet count, ensuring that vorbis doesn't try to process a header packet as a non-header packet. I discusses this approach with Conrad, and we agreed it was the best approach.

Mochitests pass locally on Windows and Linux.

Comment 2

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

(In reply to comment #1)
> Created an attachment (id=397091) [details]
> * Fixed our decode logic so that we don't call oggplay_step_decoding() after
> it's returned E_OGGPLAY_OK (it does this when it reaches EOF. This prevents
> infinite loops in oggplay_step_decoding).

This also is the same fix as in bug 496051.

Comment 3

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

(In reply to comment #2)
> This also is the same fix as in bug 496051.

Which has already got r=doublec.

Comment 4

[cajbir (:cajbir)]

Comment on attachment 397091 [details] [diff] [review]
Patch + fixes

This patch seems to remove the os2 semaphore file.

Comment 5

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

Attachment: patch v2

New and improved with less oops!

Comment 6

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

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/f4087c5ac148

Comment 7

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

D'oh! patch v2 actually still removed os2_semaphore.*. I made a mistake putting the patch together, so I've backed out, fixed and repushed.

backout/merge: http://hg.mozilla.org/mozilla-central/rev/8c409bffb344
oggplay-update fixed push: http://hg.mozilla.org/mozilla-central/rev/fb6d235b9efb

Comment 8

[Benjamin Smedberg]

This bug is suspected of causing random orange, see bug 513999 for details.

Comment 9

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

Backed out bug 512328: http://hg.mozilla.org/mozilla-central/rev/e8b3bd49771e

Comment 10

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

Is this essential for 1.9.2? I'm not sure what it fixes.

Comment 11

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

This bug fixes all the bugs it blocks: bug 487197, bug 496051, bug 499512, bug 506305, and bug 507167. Two of those bugs are security sensitive. It also makes a number of files which were previously unplayable play. I think we should take it on 1.9.2 if we can figure out what's wrong with it.

Comment 12

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

I am going to cherry pick the individual changesets required to fix the blocked bugs, and not make a monolithic liboggplay update. Hopefully we won't have the random timeout on tinderbox then, or if we do, we will have narrowed down the range of liboggplay changesets which could cause the timeout.

Comment 13

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

Looks like we need these liboggplay changesets:
17ef4ca82df28eab294fb8c41e7 for bug 504843 and bug 512328
5814d283ef6458a9b9 for bug 487197
9c7a74803736d45c10c40e4f15f89924 for bug 511038

There are several others in there that would be nice to have, but I'll start with those.

Comment 14

[Reed Loden [:reed]]

I'm just worried that by cherrypicking random patches, we'll end up missing something fixed upstream that later causes us problems (e.g., see various blocking bugs that backs this thought up). :/

Comment 15

[Bodo Graumann]

Could you please have a look over at bug 501708 and see if it can be fixed with one of the changesets? Maybe it should also be added as blocked by this one.

Comment 16

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

These files also don't play without the liboggplay update, they depend on liboggplay changeset 17ef4ca82df28.

http://myrandomnode.dyndns.org:8080/~gmaxwell/theora/nolimit.ogv
http://myrandomnode.dyndns.org:8080/~gmaxwell/theora/limit.ogv
http://myrandomnode.dyndns.org:8080/~gmaxwell/theora/limitmod.ogv

Comment 17

[Matthew Gregan [:kinetik]]

(In reply to comment #16)
> These files also don't play without the liboggplay update, they depend on
> liboggplay changeset 17ef4ca82df28.
> 
> http://myrandomnode.dyndns.org:8080/~gmaxwell/theora/nolimit.ogv
> http://myrandomnode.dyndns.org:8080/~gmaxwell/theora/limit.ogv
> http://myrandomnode.dyndns.org:8080/~gmaxwell/theora/limitmod.ogv

What's happening is that the Vorbis packets are processed in the following order:

During oggplay_initialise: header #0.
...oggplay_get_duration causes the stream to seek to offset 0...
During first oggplay_step_decoding: header #0 (again), header #1, header #2, and onwards.

Fishsound is asked to process the first header twice, which causes its internal header/packet counting to be off by one, eventually causing it to process the last header packet as a data packet, resulting in an error.

The fix is accidental in this case.  With the newer liboggplay, it processes the headers for all streams during initialize.  As all three Vorbis header packets are processed before initialize returns.  When oggplay_step_decoding is called, all three header packets are processed as data (and rejected by Vorbis) before the first data packet is processed.  fishsound's internal packet counter is incorrect, but otherwise there is no problem decoding the file.

The correct fix is Chris's patch in bug 516323, which resets fishsound's internal packet count when a seek occurs.

Comment 18

[Viktor Gal]

The (In reply to comment #17)
> The fix is accidental in this case.  With the newer liboggplay, it processes
> the headers for all streams during initialize.  As all three Vorbis header
> packets are processed before initialize returns.  When oggplay_step_decoding is
> called, all three header packets are processed as data (and rejected by Vorbis)
> before the first data packet is processed.  fishsound's internal packet counter
> is incorrect, but otherwise there is no problem decoding the file.

This - processing the headers as data - could be avoided if the byte offset value of oggz_tell would return the correct offset value. The seek-rewrite branch of oggz gives back the right offset value, but I don't know when is that going to be merge into master branch, and when is that going to be taken in into mozilla's repository. Thus, until then the only solution is the patch created by Chris in bug 516323.

Comment 19

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

I think if we're going to do this, we need to do it before beta. Otherwise we're going to have to cherrypick and/or fix the bugs this blocks independently.

Comment 20

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

Attachment: liboggplay update changesets as patch queue

liboggplay update a 1 patch per changeset. This is the contents of my .hg/patches/ directory.

Comment 21

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

Attachment: Patch v3 WIP

Patch - all the liboggplay changesets rolled up into one patch. This is the output of `hg diff -r qparent -r tip` with all patches in queue applied. Not cleaned up or ready for review.

I've basically given up on cherry picking the changesets we want, the changesets we want rely on other ones, so it's a real mess to do that...

Comment 22

[Reed Loden [:reed]]

Comment on attachment 402284 [details] [diff] [review]
Patch v3 WIP

>   * liboggz and libfishsound -- from svn.annodex.net:
> 
>-    svn co http://svn.annodex.net/liboggz/trunk liboggz
>-    svn co http://svn.annodex.net/libfishsound/trunk libfishsound
>+    git clone git://git.xiph.org/liboggz.git
>+    git clone git://git.xiph.org/libfishsound.git

s/from svn.annodex.net/from git.xiph.org/

Comment 23

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

nthomas provided us with the following stack trace from a crashing unit test machines: http://pastebin.mozilla.org/672550 

Thread 7
0  libxul.so!nsMediaCacheStream::Read(char*, unsigned int, unsigned int*) [nsMediaCache.cpp:287d383d5652 : 1934 + 0xb]
1  libxul.so!nsChannelReader::io_read(char*, unsigned int) [nsChannelReader.cpp:287d383d5652 : 65 + 0xc]
2  libxul.so!oggz_io_read [oggz_io.c:287d383d5652 : 67 + 0x9]
3  libxul.so!oggz_read [oggz_read.c:287d383d5652 : 593 + 0xc]
4  libxul.so!oggplay_initialise [oggplay.c:287d383d5652 : 125 + 0xe]
5  libxul.so!oggplay_open_with_reader [oggplay.c:287d383d5652 : 180 + 0x9]
6  libxul.so!nsOggDecodeStateMachine::LoadOggHeaders(nsChannelReader*) [nsOggDecoder.cpp:287d383d5652 : 1753 + 0x7]
7  libxul.so!nsOggDecodeStateMachine::Run() [nsOggDecoder.cpp:287d383d5652 : 1415 + 0xc]
8  libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:287d383d5652 : 527 + 0xa]
9  libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd]
10  libxul.so!nsThread::ThreadFunc(void*) [nsThread.cpp:287d383d5652 : 253 + 0x9]
11  libnspr4.so!_pt_root [ptthread.c:287d383d5652 : 228 + 0x8]
12  libpthread-2.5.so + 0x52da


Thread 0 (crashed)
 0  ld-2.5.so + 0x7f2
 1  libglib-2.0.so.0.1200.3 + 0x2b7b2
 2  libglib-2.0.so.0.1200.3 + 0x2b92c
 3  libglib-2.0.so.0.1200.3 + 0x2d741
 4  libglib-2.0.so.0.1200.3 + 0x2df14
 5  libglib-2.0.so.0.1200.3 + 0x2e7b4
 6  libxul.so!nsAppShell::ProcessNextNativeEvent(int) [nsAppShell.cpp:287d383d5652 : 147 + 0x9]
 7  libxul.so!nsBaseAppShell::DoProcessNextNativeEvent(int) [nsBaseAppShell.cpp:287d383d5652 : 151 + 0xa]
 8  libxul.so!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, int, unsigned int) [nsBaseAppShell.cpp:287d383d5652 : 278 + 0x9]
 9  libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:287d383d5652 : 508 + 0x1c]
10  libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd]
11  libxul.so!nsThread::Shutdown() [nsThread.cpp:287d383d5652 : 468 + 0xb]
12  libxul.so!nsOggDecoder::Stop() [nsOggDecoder.cpp:287d383d5652 : 1926 + 0x8]
13  libxul.so!nsRunnableMethod<nsOggDecoder, void>::Run() [nsThreadUtils.h : 264 + 0x10]
14  libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:287d383d5652 : 527 + 0xa]
15  libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd]
16  libxul.so!nsBaseAppShell::Run() [nsBaseAppShell.cpp:287d383d5652 : 170 + 0x9]
17  libxul.so!nsAppStartup::Run() [nsAppStartup.cpp:287d383d5652 : 182 + 0x5]
18  libxul.so!XRE_main [nsAppRunner.cpp:287d383d5652 : 3418 + 0x8]
19  firefox-bin!main [nsBrowserApp.cpp:287d383d5652 : 156 + 0xe]
20  libc-2.5.so + 0x15deb


The liboggplay update changes the ogg header reading code to read in a loop in oggplay_initialise(), and to check the result of oggz_read() in a switch statement. Unfortunately the switch statement doesn't include a case for OGGZ_ERR_SYSTEM, which is what's returned if the underlying stream is closed. Nor does it have a default case. So it looks like we can get stuck in a loop there reading from a closed stream, expecting to read more data, but never advancing. If a stream is closed while we're trying to load the headers, we'll hit this case, and the decode thread will not shut down. I've pushed a fixed update to tryserver, we'll find out soon if this is the only problem...

Special thanks to Nick for providing the stack trace!

Comment 24

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

(In reply to comment #22)
> (From update of attachment 402284 [details] [diff] [review])
> >   * liboggz and libfishsound -- from svn.annodex.net:
> > 
> >-    svn co http://svn.annodex.net/liboggz/trunk liboggz
> >-    svn co http://svn.annodex.net/libfishsound/trunk libfishsound
> >+    git clone git://git.xiph.org/liboggz.git
> >+    git clone git://git.xiph.org/libfishsound.git
> 
> s/from svn.annodex.net/from git.xiph.org/

This file copied directly from liboggplay upstream.

Wiking: There's an error in your README file as Reed pointed out.

Comment 25

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

Attachment: Patch v3

Patch v3, with fix for infinite loop.

Comment 26

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

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/314c78cf8700

Comment 27

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

Pushed to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6c70bde16ceb

Comment 28

[Reed Loden [:reed]]

Comment on attachment 402529 [details] [diff] [review]
Patch v3

Will this same patch work for 1.9.1, or do you need a slightly different patch?

Comment 29

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

We should wait until this patch proves itself reliable before considering it on 1.9.1. (or before resolving its dependents).

Comment 32

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

Attachment: Patch v3, backported to 1.9.1

Patch v3, backported to 1.9.1. Tests pass on Linux at least. Some tests which require the new test harness which doesn't exist on this branch are not ported across. Requesting approval 1.9.1.

This bug requires the patches in bug 512327, bug 505227, and bug 511584. Please can you approve them for 1.9.1 when you approve this one.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 403698 [details] [diff] [review]
Patch v3, backported to 1.9.1

Approved for 1.9.1.4, a=dveditz

Comment 34

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

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0a72af930207

Comment 35

[Al Billings [:abillings - ex-MoCo]]

How can QA verify this for 1.9.1? Play some ogg sound and theora video?

Comment 36

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

(In reply to comment #35)
> How can QA verify this for 1.9.1? Play some ogg sound and theora video?

Yeah, if everything works as it previously did, then we're ok.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Chris, could we collect a couple of ogg files QA should use to verify those updates? I don't have an idea how many different formats are out there we should cover with play/pause/seek or playback at all (see bug 519155 for chained ones). If we would have such a list we could easily create a Mozmill script which plays them all and does seeking to random positions. Probably we should do this offline with Tony.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Verified it based on the testing that Tony and Anthony did in QA after this fix landed (per Tony's e-mail).