New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 389219 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2014
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Use-of-uninitialized-value in WebCore::BiquadDSPKernel::updateCoefficientsIfNecessary

Project Member Reported by ClusterFuzz, Jun 26 2014

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4686250902552576

Fuzzer: Attekett_webaudio_fuzzer
Job Type: Linux_msan_chrome

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  - crash stack -
  WebCore::BiquadDSPKernel::updateCoefficientsIfNecessary
  WebCore::BiquadDSPKernel::getFrequencyResponse
  WebCore::BiquadProcessor::getFrequencyResponse
  

Minimized Testcase (1.83 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95U80z6MrWK6ndvvHB1TDunZW6-TqpTAUEoIpKL7AABpfeR5sRN_6uV4ielUFZzY1PXnnQ2F7WVJVXx0q1-Ik5E0mlQbW2EwXxvCpLQfRt3awXUBYqSfUpZ8TYR9I3peDiHoY9d--H2SZhuMWRfyNzg31MAEw
Filer: inferno@chromium.org
 
Cc: earthdok@chromium.org
Owner: rtoy@chromium.org
Status: Assigned
If !isGood, value in finalValue() never gets initialized.

float AudioParam::finalValue()
{
    float value;
    calculateFinalValues(&value, 1, false);
    return value;
}

void AudioParam::calculateFinalValues(float* values, unsigned numberOfValues, bool sampleAccurate)
{
    bool isGood = context() && context()->isAudioThread() && values && numberOfValues;
    ASSERT(isGood);
    if (!isGood)
        return;

Comment 2 by rtoy@chromium.org, Jun 26 2014

This fails because context()->isAudioThread is false.  Need to investigate why getFrequencyResponse is calling this function.
I understand that context()->isAudioThread might be the real functional bug, but to prevent this from happening in future, please also initialize float value to some default.
Project Member

Comment 4 by ClusterFuzz, Jun 26 2014

Labels: Pri-1
Project Member

Comment 5 by ClusterFuzz, Jun 29 2014

Labels: Missing_Impact-1

Comment 6 Deleted

Comment 7 by rsesek@chromium.org, Jun 30 2014

Labels: -Missing_Impact-1 Security_Impact-Beta Security_Impact-Stable
Did a CL land for this, or is ClusterFuzz mistaken?

Comment 8 by rtoy@chromium.org, Jun 30 2014

I think it's a clusterfuzz mistake. I do have a CL in the CQ for this: https://codereview.chromium.org/354213002/
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 30 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=177250

------------------------------------------------------------------
r177250 | rtoy@chromium.org | 2014-06-30T23:29:01.905735Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/webaudio/BiquadDSPKernel.cpp?r1=177250&r2=177249&pathrev=177250
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/webaudio/AudioParam.cpp?r1=177250&r2=177249&pathrev=177250
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/webaudio/BiquadDSPKernel.h?r1=177250&r2=177249&pathrev=177250

Initialize value since calculateFinalValues may fail to do so.

Fix threading issue where updateCoefficientsIfNecessary was not always
called from the audio thread. This causes the value not to be
initialized.

Thus,

o Initialize the variable to some value, just in case.
o Split updateCoefficientsIfNecessary into two functions with the code
  that sets the coefficients pulled out in to the new function
  updateCoefficients.
o Simplify updateCoefficientsIfNecessary since useSmoothing was always
  true, and forceUpdate is not longer needed.
o Add process lock to prevent the audio thread from updating the
  coefficients while they are being read in the main thread. The audio
  thread will update them the next time around.
o Make getFrequencyResponse set the lock while reading the
  coefficients of the biquad in preparation for computing the
  frequency response.

BUG= 389219 

Review URL: https://codereview.chromium.org/354213002
-----------------------------------------------------------------
Status: Fixed
Project Member

Comment 11 by ClusterFuzz, Jul 1 2014

Labels: -Restrict-View-SecurityTeam Merge-Triage M-37 M-36 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz

Comment 12 Deleted

Cc: timwillis@chromium.org
Labels: -Merge-requestd Merge-Requested
amineer@ - Merge-Requested for M37 (branch 2062).
Labels: -Merge-Requested Merge-Approved
merge approved for m37 branch 2062
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 14 2014

Labels: -Merge-Approved merge-merged-2062
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=178109

------------------------------------------------------------------
r178109 | rtoy@google.com | 2014-07-14T23:14:53.340854Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/Source/modules/webaudio/AudioParam.cpp?r1=178109&r2=178108&pathrev=178109
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/Source/modules/webaudio/BiquadDSPKernel.h?r1=178109&r2=178108&pathrev=178109
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/Source/modules/webaudio/BiquadDSPKernel.cpp?r1=178109&r2=178108&pathrev=178109

Merge 177250 "Initialize value since calculateFinalValues may fa..."

> Initialize value since calculateFinalValues may fail to do so.
> 
> Fix threading issue where updateCoefficientsIfNecessary was not always
> called from the audio thread. This causes the value not to be
> initialized.
> 
> Thus,
> 
> o Initialize the variable to some value, just in case.
> o Split updateCoefficientsIfNecessary into two functions with the code
>   that sets the coefficients pulled out in to the new function
>   updateCoefficients.
> o Simplify updateCoefficientsIfNecessary since useSmoothing was always
>   true, and forceUpdate is not longer needed.
> o Add process lock to prevent the audio thread from updating the
>   coefficients while they are being read in the main thread. The audio
>   thread will update them the next time around.
> o Make getFrequencyResponse set the lock while reading the
>   coefficients of the biquad in preparation for computing the
>   frequency response.
> 
> BUG= 389219 
> 
> Review URL: https://codereview.chromium.org/354213002

TBR=rtoy@chromium.org

Review URL: https://codereview.chromium.org/390003006
-----------------------------------------------------------------
Labels: -M-36 Release-0-M37
Labels: -reward-topanel -Security_Impact-Beta reward-unpaid reward-500
Thanks again for the fuzzer contribution! This report qualifies for a $500 reward.
Cc: attek...@gmail.com
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 19 by ClusterFuzz, Oct 7 2014

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Labels: -reward-inprocess
Processing via our e-payment system can take a few weeks, but reward should be on its way to you. Thanks again for your help!
Project Member

Comment 21 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 22 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment