723860 - Uninitalised value use in nsMediaCacheStream::GetNextCachedDataInternal

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Seward [:jseward]]

x86_64-linux, running on a 16 bit deep vnc server.

TEST_PATH= .. well, from the log output it seems like it occurs in
content/media/test/test_referer.html.  But I can't reproduce it
with that test path.  I can only reproduce with TEST_PATH=content/media/test

hence:

(DISPLAY=:1 TEST_PATH=content/media/test make -C ff-opt mochitest-plain EXTRA_TEST_ARGS='--close-when-done --debugger=vTRUNK --debugger-args="--tool=memcheck --suppressions=/home/sewardj/MOZ/fglrx-supp.supp --suppressions=/home/sewardj/MOZ/moz-supp.supp --error-limit=no --stats=yes --smc-check=all-non-file --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel' --track-origins=yes"') 2>&1 | tee spew2-memcheck-3b

Complaint is:

Conditional jump or move depends on uninitialised value(s)
   at 0x86B08D4: nsMediaCacheStream::GetNextCachedDataInternal(long) (nsMediaCache.cpp:2041)
   by 0x86B09A3: nsMediaCacheStream::GetNextCachedData(long) (nsMediaCache.cpp:1995)
   by 0x86C686E: nsWaveReader::GetBuffered(nsTimeRanges*, long) (nsWaveReader.cpp:275)
   by 0x86B77B7: nsBuiltinDecoderStateMachine::GetBuffered(nsTimeRanges*) (nsBuiltinDecoderStateMachine.cpp:2184)
   by 0x86B78EF: nsBuiltinDecoderStateMachine::NotifyDataArrived(char const*, unsigned int, unsigned int) (nsBuiltinDecoderStateMachine.cpp:1231)
   by 0x86B363E: nsMediaChannelStream::CopySegmentToCache(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (nsMediaStream.cpp:378)
   by 0x8CD0E92: nsInputStreamTee::WriteSegmentFun(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (nsInputStreamTee.cpp:223)
   by 0x8CD654E: nsPipeInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (nsPipe3.cpp:799)
   by 0x86B419C: nsMediaChannelStream::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned int) (nsMediaStream.cpp:411)
   by 0x7F470F5: nsStreamListenerTee::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) (nsStreamListenerTee.cpp:122)
   by 0x7FB0E00: nsHttpChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) (nsHttpChannel.cpp:4456)
   by 0x7F2AA3A: nsInputStreamPump::OnStateTransfer() (nsInputStreamPump.cpp:514)

 Uninitialised value was created by a heap allocation
   at 0x4C28223: malloc (vg_replace_malloc.c:263)
   by 0x67BFECB: moz_xmalloc (mozalloc.cpp:103)
   by 0x86C664F: nsWaveDecoder::CreateStateMachine() (mozalloc.h:229)
   by 0x86B71FF: nsBuiltinDecoder::Load(nsMediaStream*, nsIStreamListener**, nsMediaDecoder*) (nsBuiltinDecoder.cpp:204)
   by 0x845C408: nsHTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) (nsHTMLMediaElement.cpp:2074)
   by 0x845C7E9: nsHTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) (nsHTMLMediaElement.cpp:348)
   by 0x7FB7D60: nsHttpChannel::CallOnStartRequest() (nsHttpChannel.cpp:764)
   by 0x7FB827E: nsHttpChannel::ContinueProcessNormal(unsigned int) (nsHttpChannel.cpp:1260)
   by 0x7FB842A: nsHttpChannel::ProcessNormal() (nsHttpChannel.cpp:1197)
   by 0x7FBB2C2: nsHttpChannel::ProcessResponse() (nsHttpChannel.cpp:1099)
   by 0x7FBBB64: nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (nsHttpChannel.cpp:4164)
   by 0x7F2A837: nsInputStreamPump::OnStateStart() (nsInputStreamPump.cpp:444)

Comment 1

[Julian Seward [:jseward]]

This is hard to reproduce, but it's still around, and I get the
impression there's something ungood about it.

From adding VALGRIND_CHECK_VALUE_IS_DEFINED macros in
nsMediaCacheStream::GetNextCachedDataInternal(PRInt64 aOffset) it's
clear that it can be supplied with an uninitialised aOffset parameter
at some point.  Indeed, printing them whilst running all of
content/media/test produces small values, eg, 0, 28942, 285310.  But
at the point where aOffset is reported as uninitialised, aOffset has
the value 4209526967819254892 (== 0x3a6b3e79654b3c6c), which doesn't
sound like a valid offset.  This also removes any doubt that it might
be a false error from valgrind.

Following the trail of callers here, it seems that the value
comes from

nsWaveReader::GetBuffered(nsTimeRanges*, long) (nsWaveReader.cpp:274)

in this call

PRInt64 startOffset =
  mDecoder->GetResource()->GetNextCachedData(mWavePCMOffset);

so I am wondering if mWavePCMOffset could be uninitialised at this
point.  (Well, obviously yes; the real question is, why?)

Looking around nsWaveReader.cpp, the only place mWavePCMOffset is
assigned to is in nsWaveReader::FindDataOffset().

Comment 2

[Julian Seward [:jseward]]

A bit more digging suggests that the uninitialised mWavePCMOffset
is created by this call stack (hand edited for clarity):

Uninitialised value was created by a heap allocation
  at 0x4029B9A: malloc (vg_replace_malloc.c:263)
  by 0x403CF6B: moz_xmalloc (mozalloc.cpp:103)
  by .........  nsWaveReader::nsWaveReader(nsBuiltinDecoder*)
  by 0x69F5505: nsWaveDecoder::CreateStateMachine() (nsWaveDecoder.cpp:61)
  by 0x69E408F: nsBuiltinDecoder::Load(mozilla::MediaResource*, nsIStreamListener**, nsMediaDecoder*) (nsBuiltinDecoder.cpp:203)
  by 0x679C268: nsHTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) (nsHTMLMediaElement.cpp:2078)
  by 0x679C649: nsHTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) (nsHTMLMediaElement.cpp:347)
  by 0x632BC70: nsHttpChannel::CallOnStartRequest() (nsHttpChannel.cpp:764)
  by 0x632C18E: nsHttpChannel::ContinueProcessNormal(unsigned int) (nsHttpChannel.cpp:1260)
  by 0x632C33A: nsHttpChannel::ProcessNormal() (nsHttpChannel.cpp:1197)
  by 0x632F1D2: nsHttpChannel::ProcessResponse() (nsHttpChannel.cpp:1099)
  by 0x632FA74: nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (nsHttpChannel.cpp:4186)

nsWaveReader::nsWaveReader doesn't appear to initialise anything.
Maybe one of the other methods (nsWaveReader::Init ?)  is supposed to
do it?

Comment 3

[Matthew Gregan [:kinetik]]

It looks like GetBuffered is called before ReadMetdata (which initializes mWavePCMOffset).  The simple fix is to add a initialization check to GetBuffered and early exit.  nsGstreamerReader.cpp needs a similar fix, otherwise it looks like it'll try to use a NULL mPlayBin.  The other nsBuiltinDecoderReader subclasses already contain initialization checks:

nsOggReader.cpp:
  if (!mInfo.mHasVideo && !mInfo.mHasAudio) {
and those two are initialized to false by default and may only become true when ReadMetadata is called.

nsWebMReader.cpp:
  if (!mContext || nestegg_tstamp_scale(mContext, &timecodeScale) == -1) {
mContext will only be non-NULL after ReadMetadata.

nsRawReader.cpp doesn't implement GetBuffered.

The pluggable decoder/reader pair from bug 714408 needs to be checked too.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: patch v0

nsMediaPluginReader.cpp checks mPlugin which is initialized during ReadMetadata, so that didn't need to be fixed.  This patch fixes the Wave and Gstreamer readers, and removes a lie from the Ogg reader comments.

Comment 5

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/596ebdcb9188

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/596ebdcb9188