Closed Bug 723860 Opened 10 years ago Closed 9 years ago

Uninitalised value use in nsMediaCacheStream::GetNextCachedDataInternal

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jseward, Assigned: kinetik)

Details

(Keywords: valgrind)

Attachments

(1 file)

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)
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().
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?
Target Milestone: --- → mozilla14
Version: Trunk → Other Branch
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.
Target Milestone: mozilla14 → ---
Version: Other Branch → Trunk
Attached patch patch v0Splinter Review
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #630458 - Flags: review?(chris.double)
Attachment #630458 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/596ebdcb9188
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.