Closed Bug 498380 Opened 15 years ago Closed 15 years ago

media/liboggz/src/liboggz/oggz.c:222: oggz_close: Assertion `oggz_dlist_is_empty(oggz->packet_buffer)' failed.

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: smaug, Assigned: kinetik)

References

()

Details

Attachments

(1 file, 1 obsolete file)

This happens at least on 64 bit linux / debug when
running mochitest /tests/toolkit/content/tests/widgets/test_videocontrols_audio_direction.html

No idea if this should be ssensitive, but marking as such just in case...

#0  0x00000032d7097581 in nanosleep () from /lib64/libc.so.6
#1  0x00000032d70973a4 in sleep () from /lib64/libc.so.6
#2  0x00002aaaaaaf5245 in ah_crap_handler (signum=6)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsSigHandlers.cpp:149
#3  0x00002aaaaaaf5e08 in nsProfileLock::FatalSignalHandler (signo=6) at nsProfileLock.cpp:216
#4  <signal handler called>
#5  0x00000032d70305c5 in raise () from /lib64/libc.so.6
#6  0x00000032d7032070 in abort () from /lib64/libc.so.6
#7  0x00000032d7029a0f in __assert_fail () from /lib64/libc.so.6
#8  0x00002aaab0c218f3 in oggz_close (oggz=0x4203f60)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/media/liboggz/src/liboggz/oggz.c:222
#9  0x00002aaab0c1c63a in oggplay_close (me=0x22ad050)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/media/liboggplay/src/liboggplay/oggplay.c:794
#10 0x00002aaab0c1214c in ~nsOggDecodeStateMachine (this=0x2d11960)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/media/video/src/nsOggDecoder.cpp:727
#11 0x00002aaaab27e713 in nsRunnable::Release (this=0x2d11960) at nsThreadUtils.cpp:51
#12 0x00002aaab0c15f59 in nsCOMPtr<nsOggDecodeStateMachine>::operator= (this=0x3bd6, rhs=0x3bd6)
    at ../../../../dist/include/nsCOMPtr.h:640
#13 0x00002aaab0c12422 in nsOggDecoder::Stop (this=0x1cd1c40)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/media/video/src/nsOggDecoder.cpp:1925
#14 0x00002aaab0c15c5b in nsRunnableMethod<nsOggDecoder, void>::Run (this=0xffffffffffffffff)
    at ../../../../dist/include/nsThreadUtils.h:264
#15 0x00002aaaab2c36aa in nsThread::ProcessNextEvent (this=0x66c7a0, mayWait=1, result=0x7fffc1214a6c)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/threads/nsThread.cpp:516
#16 0x00002aaaab27e567 in NS_ProcessNextEvent_P (thread=0x3bd6, mayWait=1) at nsThreadUtils.cpp:230
#17 0x00002aaaab2c39a3 in nsThread::Shutdown (this=0x225a920)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/threads/nsThread.cpp:465
#18 0x00002aaab0c1240a in nsOggDecoder::Stop (this=0x2cd8700)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/media/video/src/nsOggDecoder.cpp:1922
#19 0x00002aaab0c15c5b in nsRunnableMethod<nsOggDecoder, void>::Run (this=0xffffffffffffffff)
    at ../../../../dist/include/nsThreadUtils.h:264
#20 0x00002aaaab2c36aa in nsThread::ProcessNextEvent (this=0x66c7a0, mayWait=1, result=0x7fffc1214b8c)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/threads/nsThread.cpp:516
#21 0x00002aaaab27e567 in NS_ProcessNextEvent_P (thread=0x3bd6, mayWait=1) at nsThreadUtils.cpp:230
#22 0x00002aaab9830392 in nsBaseAppShell::Run (this=0xe5e020)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:170
#23 0x00002aaab70c981b in nsAppStartup::Run (this=0xc40270)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:193
#24 0x00002aaaaaaeafae in XRE_main (argc=<value optimized out>, argv=<value optimized out>, aAppData=<value optimized out>)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsAppRunner.cpp:3340
#25 0x0000000000400eca in main (argc=5, argv=0x0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/browser/app/nsBrowserApp.cpp:156
Happens to me too on my 32-bit Ubuntu box.
I don't see how packet_buffer can be guaranteed to be empty in oggz_close. For example, in oggz_read_sync we have
              if (oggz_dlist_deliter(oggz->packet_buffer, oggz_read_deliver_packet) == -1) {
		return OGGZ_ERR_HOLE_IN_DATA;

This can happen if oggz_read_deliver_packet returns DLIST_ITER_ERROR:
    if (p->stream->read_packet(p->oggz, &(p->packet), p->serialno, 
			       p->stream->read_user_data) != 0) {
      return DLIST_ITER_ERROR;

So if the stream read fails, we return DLIST_ITER_ERROR, which returns -1 from oggz_dlist_deliter, which means oggz_read_sync and then oggz_read return with a non-empty oggz->packet_buffer. If the library user then calls oggz_close, this assert gets triggered.

What's the story? Is the assert really valid?
That's the result of code we changed in bug 487519. We pass down the error from the vorbis_synthesis and changed this code to propogate the error down. Possibly this has resulted in the relevance of the assertion changing. If you change anything here you should test to see if the files in bug 487519 crash.
This blocks me running mochitest test on a debug build.
(and because of that, it is hard to find a leak :( )
Flags: blocking1.9.2?
Conrad, can you help us out here? See comment #3
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Blocks: 372581
No longer blocks: 372581
Depends on: 372581
I still see this.  I'm also on 64-bit Linux.  Nut sure if anyone sees it on other platforms.
This bug needs an owner, preferably one who doesn't object to 64-bit Linux.
Assignee: nobody → kinetik
I don't think 64-bit linux is necessary to trigger this bug.  The attachment on bug 504850 (duplicate of this) should do it on 32-bit linux.
(In reply to comment #11)
> I don't think 64-bit linux is necessary to trigger this bug.  The attachment on
> bug 504850 (duplicate of this) should do it on 32-bit linux.

Thanks David. I've set that testcase as this bug's URL. I note that file has a CMML track.
Hmmm - now that I think about it, that file might only work in oggplayer (the standalone that uses the same libraries).
I don't have time right now, but when I do, I'll try to find/generate a file that works in firefox.
That file crashes for me on native 32bit Ubuntu 9.0.4 on trunk.
Attached patch patch v0 (obsolete) — Splinter Review
The crash with David's file is not platform specific, and even causes oggz-dump to hit the same assertion.  The assert doesn't make any sense to me, like Robert says, there's nothing that ensures packet_buffer is empty between oggz_read_sync and oggz_close other than the regular processing path.  The oggz_dlist_delete call immediately after the assert frees everything that may remain in the list, so the assert doesn't protect an unhandled situation.

It seems like it's possible to hit this with truncated files, otherwise malformed files, or with valid files in shutdown situations (causing the reader to return an error).

Looking at the history, the assert and delete lines were added in the same commit back in October 2007.

Attached patch removes the assert and adds David's file as an error test.
Attachment #405416 - Flags: review?(chris.double)
Attachment #405416 - Attachment is obsolete: true
Attachment #405416 - Flags: review?(chris.double)
Comment on attachment 405416 [details] [diff] [review]
patch v0

Conrad pointed out that this'll leak the underlying data.  For some reason I thought the data was part of the memory allocated with the list node.
Attached patch patch v1Splinter Review
Better patch:
 - introduces a new callback to free packet buffers (without processing them)
 - passes the new callback to oggz_dlist_deliter in oggz_close
 - existing oggz_dlist_delete frees the dlist itself

Conrad, does this look okay to you?
Attachment #405814 - Flags: review?(chris.double)
Attachment #405814 - Flags: review?(chris.double) → review+
http://hg.mozilla.org/mozilla-central/rev/0b404b342279
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #405814 - Flags: approval1.9.2?
Whiteboard: [needs review] → [needs 192 landing]
The patch looks ok, and I've applied it to liboggz upstream git master, as well as to 1.0-stable branch on github/kfish/liboggz.

When you next update liboggz, if you want to pull in the liboggz-1.1 oggz_packet changes then pull from master, otherwise just update to 1.0-stable.
Attachment #405814 - Flags: approval1.9.2?
Comment on attachment 405814 [details] [diff] [review]
patch v1

Oops, this is blocking so doesn't need explicit approval.
Whiteboard: [needs 192 landing]
We're are now crashing on Windows when loading bug498380.ogv, which was added in this bug's patch...
I checked out and built rev 0b404b342279 (the original m-c checkin for this bug) on Windows, and it crashes when loading bug498830.ogv, so I assume it never worked on Windows, and we never picked it up somehow...

It's a heap corruption ("CRT detected that the application wrote to memory after end of heap buffer."), so maybe by pure luck we're not overwriting critical stuff enough on other platforms...
If I remove disable the memory free's added by 0b404b342279, we still crash. So this crash was not caused by 0b404b342279, it's a newly discovered issue which has shown up due to the changes made here. We'll continue debugging in bug 523575...
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: