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

RESOLVED FIXED in Firefox 42

Status

()

P3
normal
Rank:
38
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jseward, Assigned: padenot)

Tracking

(Depends on: 1 bug)

Trunk
mozilla42
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8569837 [details]
Valgrind complaint (one of two)

Mochitest/Valgrind run, seen in

dom/media/tests/mochitest/test_peerConnection_noTrickleAnswer.html
(Assignee)

Updated

4 years ago
Component: Web Audio → WebRTC: Audio/Video
(Assignee)

Updated

4 years ago
Assignee: nobody → padenot
(Reporter)

Comment 1

4 years ago
Created attachment 8575432 [details] [diff] [review]
A possible fix

This stops Valgrind complaining, although it's something of a
blunt-instrument solution.
Attachment #8575432 - Flags: feedback?(padenot)
(Assignee)

Comment 2

4 years ago
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.
Attachment #8575432 - Flags: feedback?(padenot) → feedback?(rjesup)
(Reporter)

Comment 3

4 years ago
rjesup, ping?
backlog: --- → webRTC+
Rank: 38
Priority: -- → P3
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).
(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.
(Reporter)

Comment 6

3 years ago
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.
(Reporter)

Comment 7

3 years ago
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.
(Reporter)

Comment 8

3 years ago
Created attachment 8630182 [details] [diff] [review]
A better fix
Attachment #8575432 - Attachment is obsolete: true
Attachment #8575432 - Flags: feedback?(rjesup)
Attachment #8630182 - Flags: review?(rjesup)

Updated

3 years ago
Attachment #8630182 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/14c4e4aded9f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 years ago
Depends on: 1190095
You need to log in before you can comment on or make changes to this bug.