It's very similar to Bug 995289 but the source of the bug is now a negative frequency, not a high frequency. I have missed this because i have ignored the crash signature for a while.

Presumably 33 is also affected.

Can you take a look at this, padenot? Thanks.

This makes ComputeCustom work with negative frequency, and add a couple asserts for extra safety. The issue was that the index where unsigned, so it would access the array at (uint32_t)-1. Turning `periodicWaveSize` into a signed integer prevents a warning.

This is the test case, turned into a crashtest. I put it into an other patch so we can land it separately if needed (depending on the security-rating).

Comment on attachment 8446569 [details] [diff] [review] Test. r= I assume this crash happened immediately and so there is no need for reftest-wait.

Comment on attachment 8446569 [details] [diff] [review] Test. r= Probably should reference the author of the testcase.

Comment on attachment 8446568 [details] [diff] [review] r= >+ if (mPhase < 0) { >+ mPhase += periodicWaveSize; >+ } When phase is -epsilon, I assume mPhase + periodicWaveSize could round to periodicWaveSize giving j1 = periodicWaveSize. Perhaps the += periodicWaveSize can instead be performed on j1 after sampleInterpolationFactor has been calculated? >+ // Bilinear interpolation between adjacent samples in each table. If the >+ // frequency is negative, we iterate on the table backward. >+ int32_t direction = mFinalFrequency > 0 ? 1 : -1; >+ int32_t j1 = floor(mPhase); >+ int32_t j2 = j1 + direction; > float sampleInterpolationFactor = mPhase - j1; When direction is -1, j2 < j1 < mPhase. I don't understand how the interpolation works or why direction is necessary. If mPhase is periodicWaveSize - epsilon, then we want j1 = periodicWaveSize - 1 and j2 = 0, don't we? The direction in which mPhase is moving doesn't need to change how it is interpreted.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #8) > Comment on attachment 8446568 [details] [diff] [review] > r= > > >+ if (mPhase < 0) { > >+ mPhase += periodicWaveSize; > >+ } > > When phase is -epsilon, I assume mPhase + periodicWaveSize could round to > periodicWaveSize giving j1 = periodicWaveSize. > Perhaps the += periodicWaveSize can instead be performed on j1 after > sampleInterpolationFactor has been calculated? Phase is supposed to go between 0 and periodicWaveSize, so we need to wrap it here. I agree that in theory the epsilon issue can be an issue, but in practice, the numbers here (enforced at dom bindings level) make it safe. I added code to make sure. > > >+ // Bilinear interpolation between adjacent samples in each table. If the > >+ // frequency is negative, we iterate on the table backward. > >+ int32_t direction = mFinalFrequency > 0 ? 1 : -1; > >+ int32_t j1 = floor(mPhase); > >+ int32_t j2 = j1 + direction; > > > float sampleInterpolationFactor = mPhase - j1; > > When direction is -1, j2 < j1 < mPhase. > I don't understand how the interpolation works or why direction is necessary. This is kind like a wave table, but with separated tables for lower frequency data and higher frequency data. We do a bi-linear interpolation between the samples because we don't have enough values. The semantic of having a negative frequency is to iterate the table backward. When we interpolate, we need to find the next value we are going to interpolate with. When we iterate backward, it is the _previous_ value in the table, modulo periodicWaveSize. Sampling the first value with a frequency of -1 needs to interpolate between the first value of the table and the last value of the table (but with sampleInterpolationFactor == 0, so in practice this samples the last value of the table). Then, some time after, we will interpolate between the last value of the table and the second-to-last value of the table, etc. > If mPhase is periodicWaveSize - epsilon, then we want > j1 = periodicWaveSize - 1 and j2 = 0, don't we? If mPhase == periodicWaveSize - epsilon, it goes: j1 = floor(periodicWaveSize - epsilon) <=> j1 = periodicWaveSize - 1 j2 = j1 + (-1) // because we are iterating backward. <=> j2 = periodicWaveSize - 2 Indeed, we want to interpolate between the last value (that will contribute the most to the final value, because mPhase - j1 is close to 1.0), and the second-to-last value (that will contribute less). > The direction in which mPhase is moving doesn't need to change how it is > interpreted. Not sure what you mean there. We just want to interpolate with the next value, and this depends on the direction in which we are iterating on the table.

This fixes the rounding with epsilon, and makes the phase increment happen after the value sampling, so we correctly sample the first value (when mPhase == 0).

Comment on attachment 8447201 [details] [diff] [review] r= >+ mPhase = std::min(periodicWaveSize - FLT_EPSILON, mPhase); peridicWaveSize is typically > 1 and so periodicWaveSize - FLT_EPSILON -> periodicWaveSize in float arithmetic. (In reply to Paul Adenot (:padenot) from comment #9) > (In reply to Karl Tomlinson (needinfo?:karlt) from comment #8) > > >+ if (mPhase < 0) { > > >+ mPhase += periodicWaveSize; > > >+ } > > > > When phase is -epsilon, I assume mPhase + periodicWaveSize could round to > > periodicWaveSize giving j1 = periodicWaveSize. > > Perhaps the += periodicWaveSize can instead be performed on j1 after > > sampleInterpolationFactor has been calculated? > > Phase is supposed to go between 0 and periodicWaveSize, so we need to wrap > it here. It needs to be wrapped at some point, but what is the problem with doing the wrapping on j1 instead of mPhase? > I agree that in theory the epsilon issue can be an issue, but in > practice, the numbers here (enforced at dom bindings level) make it safe. mPhase is an accumulator. The increment periodicWaveSize * mFinalFrequency / mSource->SampleRate() is likely to have rounding errors, so I don't see how dom level restrictions on the numbers will prevent mPhase becoming arbitrarily close to zero. > > >+ // Bilinear interpolation between adjacent samples in each table. If the > > >+ // frequency is negative, we iterate on the table backward. > > >+ int32_t direction = mFinalFrequency > 0 ? 1 : -1; > > >+ int32_t j1 = floor(mPhase); > > >+ int32_t j2 = j1 + direction; > > > > > float sampleInterpolationFactor = mPhase - j1; > > > > When direction is -1, j2 < j1 < mPhase. > > I don't understand how the interpolation works or why direction is necessary. > > This is kind like a wave table, but with separated tables for lower > frequency data and higher frequency data. We do a bi-linear interpolation > between the samples because we don't have enough values. The semantic of > having a negative frequency is to iterate the table backward. When we > interpolate, we need to find the next value we are going to interpolate > with. When we iterate backward, it is the _previous_ value in the table, > modulo periodicWaveSize. The comment in the code says interpolation between *adjacent* samples, and that is how interpolation usually works, but the introduction of a direction here causes the samples to be both on one side of the desired phase (for one direction), an adjacent sample and the next sample beyond that. Whatever the direction, surely we want adjacent samples on each side. > > If mPhase is periodicWaveSize - epsilon, then we want > > j1 = periodicWaveSize - 1 and j2 = 0, don't we? > > If mPhase == periodicWaveSize - epsilon, it goes: > > j1 = floor(periodicWaveSize - epsilon) > <=> j1 = periodicWaveSize - 1 > > j2 = j1 + (-1) // because we are iterating backward. > <=> j2 = periodicWaveSize - 2 > > Indeed, we want to interpolate between the last value (that will contribute > the most to the final value, because mPhase - j1 is close to 1.0), and the > second-to-last value (that will contribute less). Ah, the existing interpolation seems to have j1 and j2 around the wrong way. If mPhase - j1 is close to 1.0, then j1 is further from mPhase and so its sample should contribute less to the interpolated value. If mPhase - j1 is close to zero, then j1 is closer to mPhase and so it should contribute more. The nearest value when mPhase is periodicWaveSize - epsilon should be the value for periodicWaveSize which is stored at index 0. > > The direction in which mPhase is moving doesn't need to change how it is > > interpreted. > > Not sure what you mean there. We just want to interpolate with the next > value, and this depends on the direction in which we are iterating on the > table. If the frequency is -ve then j1 is the next value. If the frequency is +ve then j2 is the next value. In each case, we also want to use the previous value, but it is easier to think in terms of adjacent values than next/previous because the concept of adjacent values need not depend on direction.

Paul, any updates here? Thanks.

Any updates here? This was filed in May and has sat around unchanged since August.

NI to mreavy as padenot is on PTO for a while. We should see if someone can update the patch to respond to the r- from karlt

Hi Karl -- Is there any chance you could update padenot's patch for this bug? And then put it up to Roc for review? It's the only "sec high" media bug that is still open, and I need to get it resolved. As Andrew says, it's been open far too long.

I'll see if I can have a look later this week, but I guess we're too late for 33 anyway.

(In reply to Karl Tomlinson (:karlt) from comment #16) > I'll see if I can have a look later this week, but I guess we're too late > for 33 anyway. No, we only just pushed b6, so next week we still have b8 and b9, so for a sec-high like this, AFAIK we should be good to take patches up to early Thursday, Oct 2.

This approach takes advantage of the fact that periodicWaveSize is a power of 2 and uses unsigned integer modulo arithmetic to find the appropriate phase of the period, even when the frequency is negative.

Comment on attachment 8497434 [details] [diff] [review] improve PeriodicWave phase-wrapping logic Review of attachment 8497434 [details] [diff] [review]: ----------------------------------------------------------------- Ah, I see. I avoided doing this originally because the spec doesn't say the input array needs to be a power of two. But periodicWaveSize here just returns the constant 4096 from the underlying implementation, and we zero-pad the coefficients out to that size. So OscillatorNode in fact always sees a power of two. Thanks for the clean-up.

Thanks for picking this up, Karl!

Comment on attachment 8497434 [details] [diff] [review] improve PeriodicWave phase-wrapping logic [Security approval request comment] How easily could an exploit be constructed based on the patch? A security bug number is enough to indicate a problem here, and it wouldn't take an interested party too long to work out what this was fixing. The flaw makes possible reads from two 16kB blocks in the lower memory adjacent to allocations of the same size. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The checkin comment is chosen so as not to spell out what was going wrong. The test is not to be landed at this stage. Which older supported branches are affected by this flaw? Versions 25 and more recent. If not all supported branches, which bug introduced the flaw? Bug 865256. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I suggest taking the fixes for bug 1074765 with this, because without that we don't have any automated test case to check correct behavior of the code. Those patches modify only the same function. If we prefer not to include fixes for bug 1074765, then a new similar patch could be created easily. How likely is this patch to cause regressions; how much testing does it need? The scope of the risk is small, limited to one Web Audio feature, which would have automated testing if bug 1074765 is fixed.

Comment on attachment 8497434 [details] [diff] [review] improve PeriodicWave phase-wrapping logic sec-approval+. We'll need this on Aurora, Beta, and ESR31. The last beta build is being made tomorrow, Oct 2, so this needs to land ASAP in order to make it. Please make Aurora, Beta, and ESR31 patches and nominate them.

Adding Sylvestre per LMandel's request.

Comment on attachment 8497434 [details] [diff] [review] improve PeriodicWave phase-wrapping logic Approval Request Comment This patch can land as is on other branches if approval is granted for the patches in bug 1074765. [Feature/regressing bug #]: bug 865256 [User impact if declined]: sec-high [Describe test coverage new/current, TBPL]: mochitest from bug 1074765 [Risks and why]: The scope of the risk is small, limited to one Web Audio feature, which would have automated testing if bug 1074765 is fixed. [String/UUID change made/needed]: none Fix Landed on Version: 35

https://hg.mozilla.org/releases/mozilla-aurora/rev/5943291a9752 https://hg.mozilla.org/releases/mozilla-beta/rev/7a21538bedac https://hg.mozilla.org/releases/mozilla-esr31/rev/8c431dcec0ff

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/efea9646666d https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/af991b3e4c9c

Confirmed crash in Fx32, ASan and release, 2014-08-26. Verified fixed in Fx33, Fx34 and Fx35, 2014-10-08. Verified fixed in Fx31.2.0esr, release candidate.