589039 - valgrind - Conditional jump or move depends on uninitialised value(s) in nsOggReader::ReadMetadata() (mSkeletonState?)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Occurs when running test_playback.html.

==28315== Thread 22:
==28315== Conditional jump or move depends on uninitialised value(s)
==28315==    at 0x57734CA: nsOggReader::ReadMetadata() (nsOggReader.cpp:323)
==28315==    by 0x5761FC7: nsBuiltinDecoderStateMachine::LoadMetadata() (nsBuiltinDecoderStateMachine.cpp:1330)
==28315==    by 0x5770209: nsOggDecoderStateMachine::LoadMetadata() (nsOggDecoderStateMachine.cpp:51)
==28315==    by 0x5763706: nsBuiltinDecoderStateMachine::Run() (nsBuiltinDecoderStateMachine.cpp:826)
==28315==    by 0x5D7F6E4: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:547)
==28315==    by 0x5D46131: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
==28315==    by 0x5D7FC1E: nsThread::ThreadFunc(void*) (nsThread.cpp:263)
==28315==    by 0x74F3F7C: _pt_root (ptthread.c:228)
==28315==    by 0x3259407760: start_thread (pthread_create.c:301)
==28315==    by 0x32590E14EC: clone (clone.S:115)

nsOggReader.cpp:323 is:
  if (mSkeletonState && mSkeletonState->HasIndex()) {

Comment 1

[Julian Seward [:jseward]]

With origin info:

Conditional jump or move depends on uninitialised value(s)
   at 0x5C688CD: nsOggReader::ReadMetadata() (nsOggReader.cpp:323)
   by 0x5C55BB5: nsBuiltinDecoderStateMachine::LoadMetadata() (nsBuiltinDecoderStateMachine.cpp:1330)
   by 0x5C65058: nsOggDecoderStateMachine::LoadMetadata() (nsOggDecoderStateMachine.cpp:51)
   by 0x5C576DB: nsBuiltinDecoderStateMachine::Run() (nsBuiltinDecoderStateMachine.cpp:826)
   by 0x6348DEF: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:547)
   by 0x6308533: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
   by 0x63493EC: nsThread::ThreadFunc(void*) (nsThread.cpp:263)
   by 0x79F4DC2: _pt_root (ptthread.c:228)
   by 0x4E359C9: start_thread (pthread_create.c:300)
   by 0xB28B6FC: clone (clone.S:112)

 Uninitialised value was created by a heap allocation
   at 0x4C285D8: malloc (vg_replace_malloc.c:236)
   by 0x73BF15B: moz_xmalloc (mozalloc.cpp:98)
   by 0x5C663A9: nsOggCodecState::Create(ogg_page*) (mozalloc.h:226)
   by 0x5C68A57: nsOggReader::ReadMetadata() (nsOggReader.cpp:203)
   by 0x5C55BB5: nsBuiltinDecoderStateMachine::LoadMetadata() (nsBuiltinDecoderStateMachine.cpp:1330)
   by 0x5C65058: nsOggDecoderStateMachine::LoadMetadata() (nsOggDecoderStateMachine.cpp:51)
   by 0x5C576DB: nsBuiltinDecoderStateMachine::Run() (nsBuiltinDecoderStateMachine.cpp:826)
   by 0x6348DEF: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:547)
   by 0x6308533: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
   by 0x63493EC: nsThread::ThreadFunc(void*) (nsThread.cpp:263)
   by 0x79F4DC2: _pt_root (ptthread.c:228)
   by 0x4E359C9: start_thread (pthread_create.c:300)
   by 0xB28B6FC: clone (clone.S:112)

Comment 2

[Julian Seward [:jseward]]

nsSkeletonState::nsSkeletonState(ogg_page* aBosPage) doesn't give
an initial value for mIndex.  Should it?

Comment 3

[Julian Seward [:jseward]]

Attachment: initialise nsSkeletonState::mIndex before use

> nsOggReader.cpp:323 is:
>   if (mSkeletonState && mSkeletonState->HasIndex()) {

== mSkeletonState->HasIndex()
   mSkeletonState->Index->Count() > 0

but afaics a nsClassHashtable doesn't have a defined Count
until it is initialised with ::Init().  Attached patch
fixes it and gets rid of the complaints.  Caveats:

1. This initialises it to the default size of 16 as
   defined in nsBaseHashtable::Init
   (xpcom/glue/nsBaseHashtable.h).  Maybe you want a
   different initial size?

2. ::Init() returns a PR_BOOL indicating success/failure,
   which is ignored.

Comment 4

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

Comment on attachment 469005 [details] [diff] [review]
initialise nsSkeletonState::mIndex before use

Thanks for working on this! I think we should add an nsSkeletonState::Init() function override, and have that return the result of mIndex.Init(). Then change the "if (mSkeletonState && mSkeletonState->HasIndex()) {...}" block in nsOggReader::ReadMetadata() to be something like:

if (mSkeletonState && mSkeletonState->Init() && mSkeletonState->HasIndex()) {
    ...
} else {
  mSkeletonState = nsnull;
}

We could probably should be nulling out mVorbisState if its Init() fails too I guess.

Comment 5

[Julian Seward [:jseward]]

Attachment: revised patch as per comment 4

> if (mSkeletonState && mSkeletonState->Init() && mSkeletonState->HasIndex()) {

I couldn't do exactly that though, at least, not _and_ remain
consistent with the style in which Init() failures for mTheoraState
and mVorbisState are handled.  So I've split this into 3 nested ifs.

> We could probably should be nulling out mVorbisState if its Init()
> fails too

Done.

Comment 6

[Julian Seward [:jseward]]

Attachment: duh, handle mVorbisState->Init() failure correctly

Comment 7

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

Comment on attachment 470461 [details] [diff] [review]
duh, handle mVorbisState->Init() failure correctly

Actually, this wasn't a good idea. Sorry for sending you down the wrong path! If I apply and run this patch, I get assertion failures:

4516[9b0d0c0]: ###!!! ASSERTION: nsTHashtable::Init() should not be called twice.: 'Error', file c:\
users\cpearce\src\purple\objdir\dist\include\nsTHashtable.h, line 327
###!!! ASSERTION: nsTHashtable::Init() should not be called twice.: 'Error', file c:\users\cpearce\s
rc\purple\objdir\dist\include\nsTHashtable.h, line 327

We init mIndex the first time we encounter something to put into it (in nsSkeletonState::DecodeHeader()), so we shouldn't re-init it in nsOggReader::ReadMetadata(). We should just have nsSkeletonState::HasIndex() return (mIndex.IsInitialized() && mIndex.Count() > 0), that way we won't call mIndex.Count() if it's not initialized. Is that sufficient to prevent the valgrind warning?

That's probably all the change we actually need, though we should still null out mVorisState if its init fails.

Comment 8

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

(In reply to comment #7)
> Comment on attachment 470461 [details] [diff] [review]
>
> That's probably all the change we actually need, though we should still null
> out mVorisState if its init fails.

I've realised while talking to Chris Double about another bug in nsOggReader today that we don't actually want to null out the mVorbisState even if its initialization fails, else the our Ogg GetBuffered() implementation could get messed up in chained oggs. We shouldn't null out mVorbisState, the only change we need here is to add the mIndex.IsInitialized() call to HasIndex().

Comment 9

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

Attachment: Patch: Only use hashtable if it's been initialized

I should really be the one fixing this, since it's in my code. This patch ensures we don't query the hashtable if it's not been initialized...

Comment 10

[Julian Seward [:jseward]]

(In reply to comment #9)
Just re-verified:
1. bug still exists in m-c of 29 Oct
2. the patch in comment 9 fixes it.

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/1ecee71d770b