1137169 - Uninitialised value uses related to mozilla::dom::WebAudioUtils::SpeexResamplerProcess

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P3)

Description

[Julian Seward [:jseward]]

Attachment: Valgrind complaint (one of two)

Mochitest/Valgrind run, seen in

dom/media/tests/mochitest/test_peerConnection_noTrickleAnswer.html

Comment 1

[Julian Seward [:jseward]]

Attachment: A possible fix

This stops Valgrind complaining, although it's something of a
blunt-instrument solution.

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8575432 [details] [diff] [review]
A possible fix

Review of attachment 8575432 [details] [diff] [review]:
-----------------------------------------------------------------

Randell, does that make sense to you ? I'm afraid that it's too far from the MSG for me to know what to do here.

Comment 3

[Julian Seward [:jseward]]

rjesup, ping?

Comment 4

[Maire Reavy [:mreavy]]

I've looked at this some.   I'm not sure how uninitialized gets to be flagged here... And I really don't want to spend the cycles to just quiet valgrind.  I'll check if I had a possible patch for it (not on that machine ATM).

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #4)
> I've looked at this some.   I'm not sure how uninitialized gets to be
> flagged here... And I really don't want to spend the cycles to just quiet
> valgrind.  I'll check if I had a possible patch for it (not on that machine
> ATM).

FWIW, Valgrind is usually right about these things.

Comment 6

[Julian Seward [:jseward]]

I looked at 

int
WebAudioUtils::SpeexResamplerProcess(SpeexResamplerState* aResampler,
                                     uint32_t aChannel,
                                     const int16_t* aIn, uint32_t* aInLen,
                                     int16_t* aOut, uint32_t* aOutLen)

at dom/media/webaudio/WebAudioUtils.cpp:88.  Almost all of the time,
*aInLen == *aOutLen == 441.  But for 4 out of at least several hundred
cases, those two have the value 882 instead, and it is exactly for these
4 that Valgrind complains.

I notice that the two arrays tmp1 and tmp2 are given initial sizes
of 512 (that is, WEBAUDIO_BLOCK_SIZE * 4), which is > 441, in which
cases Valgrind does not complain, and < 882, for which it does 
complain.  So I wonder if that is somehow relevant?  But the two
arrays are resized to the actual required size before use, so at
least in this function I don't see anything obviously wrong.

Comment 7

[Julian Seward [:jseward]]

Thanks to Randell for helping me make sense of this.

This is caused by a confusion about number-of-bytes vs number-of-samples
in the error case in MediaPipelineReceiveAudio::PipelineListener::NotifyPull.

If the call to GetAudioFrame fails, then err will be != kMediaConduitNoError
and specifically will be kMediaConduitSessionNotInited.  It then enters
here

    if (err != kMediaConduitNoError) {
      // Insert silence on conduit/GIPS failure (extremely unlikely)

and computes into samples_length, the number of bytes of samples_data
that need to be zeroed out.  But further down, samples_length is handed
to segment.AppendFrames(..) to mean the number of samples to add.  The
effect is that subsequent processing overruns the zeroed out area
because the size of a sample is 16 bits.

Comment 8

[Julian Seward [:jseward]]

Attachment: A better fix

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14c4e4aded9f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14c4e4aded9f