921622 - Intermittent ASAN heap-buffer-overflow in test_playback_rate.html

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this doesn't fail in a TBPL-starrable way. Decoder, anything we can do about that?

https://tbpl.mozilla.org/php/getParsedLog.php?id=28460670&tree=Mozilla-Inbound

Ubuntu ASAN VM 12.04 x64 mozilla-inbound opt test mochitest-1 on 2013-09-27 04:07:56 PDT for push 06f9ccafcf8b
slave: tst-linux64-ec2-373

04:43:57     INFO -  153499 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | If we disable the pitch preservation, it should appear as such.
04:43:57     INFO -  153500 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | The playback rate shoud be 0.25.vbr.mp3-5
04:43:57     INFO -  153501 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | We are effectively slowing down playback. (18.025, 4.217999) for vbr.mp3-5
04:43:57     INFO -  153502 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | Current time should not change when playbackRate is null (4.217999 4.217999).
04:43:57     INFO -  153503 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | The element should not be in paused state.
04:43:57     INFO -  153504 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | Paused event should not have been received.
04:43:57     INFO -  153505 INFO TEST-PASS | /tests/content/media/test/test_playback_rate.html | Playback rate should be clamped to 5.
04:43:58     INFO -  =================================================================
04:43:58     INFO -  ==2421==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f46b7bbe810 at pc 0x7f46f1bc4ad3 bp 0x7f46b04d1250 sp 0x7f46b04d1248
04:43:58     INFO -  WRITE of size 4 at 0x7f46b7bbe810 thread T85
04:43:58     INFO -      #0 0x7f46f1bc4ad2 (/builds/slave/test/build/application/firefox/libxul.so+0x63a7ad2)
04:43:58     INFO -      #1 0x7f46f1bc2c6e (/builds/slave/test/build/application/firefox/libxul.so+0x63a5c6e)
04:43:58     INFO -      #2 0x7f46f1bc5f95 (/builds/slave/test/build/application/firefox/libxul.so+0x63a8f95)
04:43:58     INFO -      #3 0x7f46ee298ab4 (/builds/slave/test/build/application/firefox/libxul.so+0x2a7bab4)
04:43:58     INFO -      #4 0x7f46ee298e36 (/builds/slave/test/build/application/firefox/libxul.so+0x2a7be36)
04:43:58     INFO -      #5 0x7f46f1bcc7a4 (/builds/slave/test/build/application/firefox/libxul.so+0x63af7a4)
04:43:58     INFO -      #6 0x44ccc3 (/builds/slave/test/build/application/firefox/firefox+0x44ccc3)
04:43:58     INFO -      #7 0x7f46fd0c4e99 (/lib/x86_64-linux-gnu/libpthread.so.0+0x7e99)
04:43:58     INFO -      #8 0x7f46fc1d5dbc (/lib/x86_64-linux-gnu/libc.so.6+0xf3dbc)
04:43:58     INFO -  0x7f46b7bbe810 is located 0 bytes to the right of 155664-byte region [0x7f46b7b98800,0x7f46b7bbe810)
04:43:58     INFO -  freed by thread T1362 (Media Audio) here:
04:43:58     INFO -      #0 0x446aa5 (/builds/slave/test/build/application/firefox/firefox+0x446aa5)
04:43:58     INFO -      #1 0x7f46f1bc0208 (/builds/slave/test/build/application/firefox/libxul.so+0x63a3208)
04:43:58     INFO -  previously allocated by thread T1362 (Media Audio) here:
04:43:58     INFO -      #0 0x4467f5 (/builds/slave/test/build/application/firefox/firefox+0x4467f5)
04:43:58     INFO -      #1 0x7f46f1bc0095 (/builds/slave/test/build/application/firefox/libxul.so+0x63a3095)
04:43:58     INFO -  Thread T85 created by T83 (Media Audio) here:
04:43:58     INFO -      #0 0x4375c1 (/builds/slave/test/build/application/firefox/firefox+0x4375c1)
04:43:58     INFO -      #1 0x7f46f1bcb061 (/builds/slave/test/build/application/firefox/libxul.so+0x63ae061)
04:43:58     INFO -      #2 0x7f46f1bca87f (/builds/slave/test/build/application/firefox/libxul.so+0x63ad87f)
04:43:58     INFO -      #3 0x7f46ee2e1b18 (/builds/slave/test/build/application/firefox/libxul.so+0x2ac4b18)
04:43:58     INFO -  Thread T83 (Media Audio) created by T76 (Media State) here:
04:43:58     INFO -      #0 0x4375c1 (/builds/slave/test/build/application/firefox/firefox+0x4375c1)
04:43:58     INFO -      #1 0x7f46f9ba6b35 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6ab35)
04:43:58     INFO -      #2 0x7f46f9ba6687 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6a687)
04:43:58     INFO -  Thread T76 (Media State) created by T0 here:
04:43:58     INFO -      #0 0x4375c1 (/builds/slave/test/build/application/firefox/firefox+0x4375c1)
04:43:58     INFO -      #1 0x7f46f9ba6b35 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6ab35)
04:43:58     INFO -      #2 0x7f46f9ba6687 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6a687)
04:43:58     INFO -  Thread T1362 (Media Audio) created by T1335 (Media State) here:
04:43:58     INFO -      #0 0x4375c1 (/builds/slave/test/build/application/firefox/firefox+0x4375c1)
04:43:58     INFO -      #1 0x7f46f9ba6b35 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6ab35)
04:43:58     INFO -      #2 0x7f46f9ba6687 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6a687)
04:43:58     INFO -  Thread T1335 (Media State) created by T0 here:
04:43:58     INFO -      #0 0x4375c1 (/builds/slave/test/build/application/firefox/firefox+0x4375c1)
04:43:58     INFO -      #1 0x7f46f9ba6b35 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6ab35)
04:43:58     INFO -      #2 0x7f46f9ba6687 (/builds/slave/test/build/application/firefox/libnspr4.so+0x6a687)
04:43:58     INFO -  Shadow bytes around the buggy address:
04:43:58     INFO -    0x0fe956f6fcb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
04:43:58     INFO -    0x0fe956f6fcc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
04:43:58     INFO -    0x0fe956f6fcd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
04:43:58     INFO -    0x0fe956f6fce0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
04:43:58     INFO -    0x0fe956f6fcf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
04:43:58     INFO -  =>0x0fe956f6fd00: fd fd[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa
04:43:58     INFO -    0x0fe956f6fd10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:43:58     INFO -    0x0fe956f6fd20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:43:58     INFO -    0x0fe956f6fd30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:43:58     INFO -    0x0fe956f6fd40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:43:58     INFO -    0x0fe956f6fd50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:43:58     INFO -  Shadow byte legend (one shadow byte represents 8 application bytes):
04:43:58     INFO -    Addressable:           00
04:43:58     INFO -    Partially addressable: 01 02 03 04 05 06 07
04:43:58     INFO -    Heap left redzone:     fa
04:43:58     INFO -    Heap right redzone:    fb
04:43:58     INFO -    Freed heap region:     fd
04:43:58     INFO -    Stack left redzone:    f1
04:43:58     INFO -    Stack mid redzone:     f2
04:43:58     INFO -    Stack right redzone:   f3
04:43:58     INFO -    Stack partial redzone: f4
04:43:58     INFO -    Stack after return:    f5
04:43:58     INFO -    Stack use after scope: f8
04:43:58     INFO -    Global redzone:        f9
04:43:58     INFO -    Global init order:     f6
04:43:58     INFO -    Poisoned by user:      f7
04:43:58     INFO -    ASan internal:         fe
04:43:58     INFO -  ==2421==ABORTING

Comment 1

[Christian Holler (:decoder)]

Okay, looking at the log there are two strange things:

1) The harness doesn't show it as an error (it doesn't even show that the process exited).
2) The log is unsymbolized although it should be symbolized.

A wild guess would be that this is not the main process but some sub-process that dies here. But I don't know the code/test well enough. Needinfo from Paul to answer this question :)

Comment 2

[Paul Adenot (:padenot)]

This is the main process.

Comment 3

[Christian Holler (:decoder)]

Here's the interesting part of the symbolized trace:

==2421==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f46b7bbe810 at pc 0x7f46f1bc4ad3 bp 0x7f46b04d1250 sp 0x7f46b04d1248
WRITE of size 4 at 0x7f46b7bbe810 thread T85
    #0 0x7f46f1bc4ad2 in RateTransposerFloat::transposeStereo(float*, float const*, unsigned int) build/media/libsoundtouch/src/RateTransposer.cpp:611
    #1 0x7f46f1bc2c6e in soundtouch::RateTransposer::transpose(float*, float const*, unsigned int) build/media/libsoundtouch/src/RateTransposer.cpp:319
    #2 0x7f46f1bc5f95 in soundtouch::FIFOSamplePipe::moveSamples(soundtouch::FIFOSamplePipe&) build/media/libsoundtouch/src/FIFOSamplePipe.h:88
    #3 0x7f46ee298ab4 in mozilla::BufferedAudioStream::GetTimeStretched(void*, long) build/content/media/AudioStream.cpp:850
    #4 0x7f46ee298e36 in mozilla::BufferedAudioStream::DataCallback(void*, long) build/content/media/AudioStream.cpp:875
    #5 0x7f46f1bcc7a4 in alsa_refill_stream build/media/libcubeb/src/cubeb_alsa.c:306
    #6 0x44ccc3 in ThreadStart _asan_rtl_
    #7 0x7f46fd0c4e99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
    #8 0x7f46fc1d5dbc in __opensock /build/buildd/eglibc-2.15/socket/../sysdeps/unix/sysv/linux/opensock.c:82
0x7f46b7bbe810 is located 0 bytes to the right of 155664-byte region [0x7f46b7b98800,0x7f46b7bbe810)
freed by thread T1362 (Media Audio) here:
    #0 0x446aa5 in operator delete[] _asan_rtl_
    #1 0x7f46f1bc0208 in soundtouch::FIFOSampleBuffer::ensureCapacity(unsigned int) build/media/libsoundtouch/src/FIFOSampleBuffer.cpp:185
previously allocated by thread T1362 (Media Audio) here:
    #0 0x4467f5 in operator new[] _asan_rtl_
    #1 0x7f46f1bc0095 in soundtouch::FIFOSampleBuffer::ensureCapacity(unsigned int) build/media/libsoundtouch/src/FIFOSampleBuffer.cpp:174
Thread T85 created by T83 (Media Audio) here:
    #0 0x4375c1 in __interceptor_pthread_create _asan_rtl_
    #1 0x7f46f1bcb061 in alsa_init build/media/libcubeb/src/cubeb_alsa.c:692
    #2 0x7f46f1bca87f in cubeb_init build/media/libcubeb/src/cubeb.c:125
    #3 0x7f46ee2e1b18 in mozilla::MediaDecoderStateMachine::AudioLoop() build/content/media/MediaDecoderStateMachine.cpp:1062

Comment 4

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28584383&tree=Mozilla-Inbound

Comment 5

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28862166&tree=Mozilla-Inbound

Comment 6

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28903669&tree=Fx-Team

Comment 7

[Mats Palmgren (inactive)]

Looks potentially exploitable to me.

Comment 8

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=29117031&tree=Mozilla-Central

Comment 9

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=29246870&tree=B2g-Inbound

Comment 10

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=29972503&tree=Mozilla-Central

Comment 11

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=29995720&tree=Mozilla-Inbound

Comment 12

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=30026163&tree=Fx-Team because I'm tired of filing new bugs for all the places you can crash after corrupting memory.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Relevant portion of a log for posterity

Comment 14

[Phil Ringnalda (:philor)]

Disabled in https://hg.mozilla.org/mozilla-central/rev/3dd1dc64123a, since as bug 931344 comment 10 notes, if you're corrupting memory in your test, the best thing that can be hoped for is random crashes in your test (like, say, bug 916734, bug 918417, bug 920827, bug 923996, bug 928225, bug 929521, bug 930982, bug 932193, bug 840742), while the worst is that a completely unknowable number of failures in other tests in any other test in mochitest-1 are the result of this.

Comment 15

[Daniel Veditz [:dveditz]]

Is Sept 27 when this started happening? Can we take that as a regression range or is the crash too intermittent to trust?

Comment 16

[Phil Ringnalda (:philor)]

It was failing every 3-10 days, and we started running ASan tests on on-push builds around 10 days before this was filed, so the regression window right now is "between when the test first landed a year ago and early September 27th" though the explosion in the apparently-random crash bugs noted in comment 14 makes early-to-mid-September look rather likely.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Phil Ringnalda (:philor) from comment #16)
> It was failing every 3-10 days, and we started running ASan tests on on-push
> builds around 10 days before this was filed, so the regression window right
> now is "between when the test first landed a year ago and early September
> 27th" though the explosion in the apparently-random crash bugs noted in
> comment 14 makes early-to-mid-September look rather likely.

Marking this as affected on 26 and tracking in that case since 26 was on nightly then.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Paul - do you have any ideas here on what our options are for FF26? We're two betas out from release and would consider a low risk backout to known good state if you can find the regressing bug.

Comment 19

[Paul Adenot (:padenot)]

I've started trying to find the reason for this, without success (yet). I'm hoping to have more luck with more machine (OS/arch) when I'm back at my desk in Paris (I'm coming back from NZ at the moment), because this is not obvious to me, and seems quite hard to repro.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Well here's hoping the difficulty to reproduce works to our advantage since we're now missing the ship date for FF26, continuing to track for FF27.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Paul, have you found the reason yet?

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

I assume this affects 29.

Comment 23

[Paul Adenot (:padenot)]

So, I've been trying to repro this for a couple months, now, no luck. I've tried windows 7, windows 8, various 32bits and 64bits linux, and osx.

I've been carefully re-reading a bunch of code, valgrinded it, ran asan builds under various load scenario, but no luck yet.

I'm pretty sure this is bad rounding from float to integer when calculating in a buffer size somewhere, but I can't find exactly where.

I'm definitely willing to fix this, but it's hard to repro, and I've been lacking the necessary time, lately.

Comment 24

[Mats Palmgren (inactive)]

Attachment: ASAN stack for "attempting to call malloc_usable_size() for pointer which is not owned"

When I run this in my Linux64 ASAN build it crashes quite quickly:
./mach mochitest-plain --debugger=gdb --repeat=1000 content/media/test/test_playback_rate.html

Not sure if this is related to the original problem though.
I can debug this further if someone tells me what to look for.

Comment 25

[Paul Adenot (:padenot)]

Mats, this is an unrelated bug. In fact, we should fix it.

Comment 26

[Mats Palmgren (inactive)]

(I filed bug 960160 on the unrelated malloc_usable_size() bug.)

Comment 27

[bhavana bajaj [:bajaj]]

We're past the time for speculative fixes on beta, so this is going to be a wontfix there but still tracking for potential forward fix if low-risk enough to take to FF28

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Paul - are you working on something for this issue?  We missed shipping in FF27 but there's still some time on 28 beta to try for a low risk fix.

Comment 29

[Paul Adenot (:padenot)]

Give me time, and I'll try to fix it, but apparently that's not going to happen.

Comment 30

[Maire Reavy [:mreavy]]

I just talked to Paul, and he simply can't repro this. And it's not a good use of his time to try at this point.  

If someone can repro this for us, narrow the range that this was introduced, or get us some useful, actionable info, we can take a look at this again.

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Matt - can you do any investigation here into reproducing and finding a regression range before we give up on this for the short term?

Comment 32

[Matt Wobensmith [:mwobensmith][:matt:]]

I've tried to reproduce this on both a build from the reported date and with two recent builds. The bug itself is light on details except that it happens during the mochitest run on this particular case. So, that's all I've had to work with.

I can't reproduce, and without a reproduction, a regression range is not possible. 

I'd say we give up for now, but I'm curious to know if this still happens at all on tbpl.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

We'd have to re-enable it to find out. Someone could push it to Try if they wanted to.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Not sure which suite it'll run in due to chunking, but I'll retrigger the relevant runs once they finish.
https://tbpl.mozilla.org/?tree=Try&rev=80fe3b4cf648

Comment 35

[Ryan VanderMeulen [:RyanVM]]

(In reply to Matt Wobensmith from comment #32)
> I'd say we give up for now, but I'm curious to know if this still happens at
> all on tbpl.

Yup, but not without a lot of effort:
https://tbpl.mozilla.org/php/getParsedLog.php?id=34931318&tree=Try

What I also learned in this experiment - this test is horribly assertion-prone on debug builds.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #35)
> Yup, but not without a lot of effort:

As in, 1/200 ASAN runs hit it.

Comment 37

[Paul Adenot (:padenot)]

Hm, indeed, I can probably fix this assertion, I seem to recall I even filed the bug and had half a patch written for it.

Anyways, it seems to crash in the linear stereo linear interpolation routine. The error is not obvious, but the stop condition seems pretty error-prone. Considering i is probably equal to the size of the buffer + 1, we could simply do something like an emergency bailout when we reach the end of the buffer. It might not sound nice (tbpl probably does not care), but surely it is better than a crash.

Then maybe we can read the code to find why that happens, and write a real fix.

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

Regarding the ASan complaint about malloc_usable_size: See bug 960160 comment 1. ASan bug, fixed in Clang 3.4, known and easy work-around.

Comment 39

[Mats Palmgren (inactive)]

Fwiw, I ran test_playback_rate.html 310 times in loop without crashing in a local
Linux64 ASAN Opt build (built with Clang 3.4).

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Paul, any chance you could fix that for 29? FYI, beta 3 is going to be built today.

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Discussed IRL with Paul. Tagging as wontfix for 29. Too hard to reproduce and only occurs in ASAN.
If people agree on that, we could untrack for the other releases...

Comment 42

[Christian Holler (:decoder)]

Why is "only occurs in ASan" as reason for not tracking? Some critical security bugs only show up in ASan. An OOB write does not necessarily show up as a regular crash, but can still be exploited in many cases.

If the test is disabled right now for some reason, then we should re-enable it and see if it's still happening.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Like the Try run I did about a month ago in comment 35? Do we have reason things have changed since?

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

This would be good to retry as soon as bug 919215 lands.  That and other bugs have refactored (and added locks) to this code.

Comment 45

[Paul Adenot (:padenot)]

Also because we have landed an update to soundtouch.

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Given my and Paul's comments, re-trying
https://tbpl.mozilla.org/?tree=Try&rev=2f359913b65c

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

annoying parser
https://tbpl.mozilla.org/?tree=Try&rev=5ee6e8a11a7e

The test seems to be perma-orange now, which may need resolving before we can verify things:
 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback_rate.html | We are effectively slowing down playback. (0.505, 0.300104) for bug495794.ogg-6

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

If I had to guess from the code (pre-update) and the ASAN stack, I'd guess that something grabbed ptrBegin() (raw ptr to the output buffer) before a call to ensureCapacity() cause it to be reallocated.

ensureCapacity() (old rev) gets called from putSamples(int) or ptrEnd().  ptrEnd returns a raw ptr to the buffer, but also uses ensureCapacity to guarantee there are 'slackCapacity' bytes available.  Unless that value was 0, ptrEnd() on it's own couldn't be the problem.

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

Also, this was a write, so most likely it was an output pointer, which would point towards ptrEnd() (which is used for writing generally); perhaps it wrote "just enough" to cause ensureCapacity() to reallocate.

Comment 50

[Randell Jesup [:jesup] (needinfo me)]

With some fixes to get test_playback_rate to run:
https://tbpl.mozilla.org/?tree=Try&rev=fb8e57d0cf08

And we still have an ASAN hit there:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38224491&tree=Try&full=1#error0

I'll attach the ASAN hit.  It does look like ptrEnd() is involved

Comment 51

[Randell Jesup [:jesup] (needinfo me)]

Attachment: asan_hit

Comment 52

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Lock before changing RateTransposer

Comment 53

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Lock before changing RateTransposer

audited the use of the Clock and TimeStretcher; found another case and cleaned up the code

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8410314 [details] [diff] [review]
Lock before changing RateTransposer

Requesting approval for all affected active branches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not trvially, but it is a UAF.  If you constantly change the rate it would happen far faster.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  The comments definitely paste a bullseye on it.  The code itself will be rather more obtuse, but someone could infer the general problem if they're watching for apparent sec-patches and especially for lock changes.

Which older supported branches are affected by this flaw? 22 and on.

If not all supported branches, which bug introduced the flaw? bug 495040 and probably bug 852401 (FF 22) which turned off sydney audio.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Backports should be simple since this code hasn't really changed, and we just need to grab the lock.

How likely is this patch to cause regressions; how much testing does it need?  Little chance of regressions (only type of regression from this would be a deadlock, but that appears impossible).

We're may want to crash-land this across as many revs as we can (with comments removed or minimized), and perhaps do a ESR 24 release.  The open question is how exploitable is it, and how much data can leak.  It is a UAF, but the data probably doesn't affect the code flow if it grabs the wrong data when reading - but it writes as well, and we have ASAN stacks showing it writing to memory outside the buffer (or likely freed memory).  It also may allow some private data to escape, but the data would be subject to possibly-reversible transforms.

We may want to at least consider a 28 chemspill, though with 29 going out in a few days it may not make sense.

Comment 55

[Lukas Blakk [:lsblakk] use ?needinfo]

This does not look like a chemspill worthy fix.  It's been in the code since FF22 without known exploit, and it's not trivial to construct one.  

We've already gone to build on the FF29 RC which will get testing on the beta audience.  If we did a build 2 for this, we lose that testing and given how big a deal the FF29 release is - we need that testing.  I'm not leaning towards losing it for a bug that has been wontfixed this many times.

If we can hold out, land this in the last weeks of FF30 across all branches - unless there was an exploit that demanded a chemspill based on actual evidence this had been found - we can make sure FF29 gets the chance to be a stable release.

Comment 56

[Al Billings [:abillings - ex-MoCo]]

This was discussed with release management. While it is old, it isn't a chemspill because we don't have a zero day. We just built the Firefox 29 release candidate so this is very late to try to take it and would cause a rebuild. 

We want to land this in the middle of the next release cycle, on May 20. This will give it enough bake time without a large window of exposure. I'm marking it for sec-approval but please don't land before then.

Comment 57

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8410314 [details] [diff] [review]
Lock before changing RateTransposer

I'm minusing this for ESR24 based on the "disabled" flag there. If this isn't disabled on ESR24, we can revisit.

Comment 58

[Randell Jesup [:jesup] (needinfo me)]

Ok. I'm trying to see if there's a way to make it happen a lot, and how dangerous that looks.

Comment 59

[Randell Jesup [:jesup] (needinfo me)]

See also bug 902328 (related); I have an expanded patch cleaning this up more.  I've been unable to find a way to get it to fail easily or locally, which is good. :-)

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Force it to run test_playback_rate a lot NOT FOR CHECKIN

Comment 61

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8412182 [details] [diff] [review]
Force it to run test_playback_rate a lot NOT FOR CHECKIN

This test will cycle enough on the testcase to get a hit in around 1/10 runs (and a higher chance of timeout, since we're running near the edge of total time allowed).  A lot better than 1/100 or 1/200 :-)

https://tbpl.mozilla.org/?tree=Try&rev=6f916a996524 has two hits in 21
https://tbpl.mozilla.org/?tree=Try&rev=724d8f12331c with the latest fix patch has no hits in about 40 runs.

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

Attachment: all.patch

Updated patch used for verification (cleaned up Internal/Unlocked usage).  Working on Aurora 30 version.

Comment 63

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8412232 [details] [diff] [review]
all.patch

Re-review of now-tested patch.

Cleans up the locking on the AudioClock functions as well.

Comment 64

[Paul Adenot (:padenot)]

Comment on attachment 8412232 [details] [diff] [review]
all.patch

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

Thanks!

::: content/media/AudioStream.cpp
@@ +227,5 @@
>  {
>    NS_ASSERTION(aPlaybackRate > 0.0,
>                 "Can't handle negative or null playbackrate in the AudioStream.");
>    // Avoid instantiating the resampler if we are not changing the playback rate.
> +  // GetPreservesPitch/SetPreservesPitch don't need locking

This comments seems off, since you are explicitly locking in SetPreservesPitch.

@@ +1070,1 @@
>  {

mAudioStream->mMonitor.AssertCurrentThreadOwns();

@@ +1107,1 @@
>  {

mAudioStream->mMonitor.AssertCurrentThreadOwns();

Comment 65

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8410314 [details] [diff] [review]
Lock before changing RateTransposer

Approving for landing this week.

Comment 66

[Andrew McCreight [:mccr8]]

Randell, this is okay to land on inbound now.

Comment 67

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Adenot (:padenot) from comment #64)

> @@ +1070,1 @@
> >  {
> 
> mAudioStream->mMonitor.AssertCurrentThreadOwns();
> 
> @@ +1107,1 @@
> >  {
> 
> mAudioStream->mMonitor.AssertCurrentThreadOwns();

a) monitor is not accessible there; I'd need to make it protected
b) these are both followed by a ...Unlocked() call that has a monitor assert as the first statement.  Added a comment instead

Comment 68

[Randell Jesup [:jesup] (needinfo me)]

Attachment: AudioStream rework for Beta

For Beta

Comment 69

[Randell Jesup [:jesup] (needinfo me)]

Attachment: AudioStream rework

For inbound and aurora

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21d19c772468

Comment 71

[Randell Jesup [:jesup] (needinfo me)]

Note: needs aurora and beta checkins as well

Comment 72

[Andrew McCreight [:mccr8]]

There's aurora and beta approvals in comment 65, for a version of the patch that was obsoleted.  I'm needinfo'ing Ryan in case that causes it to not show up in his queries.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Shouldn't be an issue. I go off the status flags and bug resolution only.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21d19c772468

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d94b8f7db6c5
https://hg.mozilla.org/releases/mozilla-beta/rev/eaa2b716ce89

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a4aba76c789b

Needs some love for b2g26.

Comment 77

[Randell Jesup [:jesup] (needinfo me)]

On beta, m1 is toasty, on a test that's been removed from nightly (and probably Aurora): test_audiowrite.html

Assertion failure: lock->locked && pthread_equal(lock->owner, pthread_self()), at /builds/slave/m-beta-l64-d-00000000000000000/build/nsprpub/pr/src/pthreads/ptsynch.c:226
15:10:52     INFO -  Assertion failure: lock->locked && pthread_equal(lock->owner, pthread_self()), at /builds/slave/m-beta-l64-d-00000000000000000/build/nsprpub/pr/src/pthreads/ptsynch.c:226

Comment 78

[Ryan VanderMeulen [:RyanVM]]

On b2g28, there are also crashes in test_autoplay.html

Comment 79

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Extend fix to AudioData-only function

Comment 80

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8426726 [details] [diff] [review]
Extend fix to AudioData-only function

This is a one-line fix (plus a function rename to match the others -- XxxUnlocked() means you already hold the monitor (ok, it's weird).

This is used by the AudioData API which is gone in later builds.  This just extends the existing fix in this bug to cover this function as well.

Comment 81

[Randell Jesup [:jesup] (needinfo me)]

I'd previously noted that it appears this is broken since cubeb replaced sydneyaudio in 22; so esr24 should be affected.  However, it's hard to be 100% sure without going back and testing/examining the code.

Comment 82

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8426726 [details] [diff] [review]
Extend fix to AudioData-only function

-> padenot; we'll back out of beta for tonight

Comment 83

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta and b2g28.

https://hg.mozilla.org/releases/mozilla-beta/rev/b7913b826440
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a61b65a57289

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

Note: the function patched on beta is dead code on nightly (and almost certainly aurora); I'll followup with a patch to remove it, so there's no need to fix on nightly.

Comment 85

[Ryan VanderMeulen [:RyanVM]]

Re-landed on beta and b2g28 with the follow-up fix folded in.

https://hg.mozilla.org/releases/mozilla-beta/rev/3090db8c413f
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/390be296492d

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3090db8c413f

Comment 87

[Lukas Blakk [:lsblakk] use ?needinfo]

If this can land cleanly to ESR24 we might as well take it.

Comment 88

[Ryan VanderMeulen [:RyanVM]]

needinfo? on the remaining branch patch(es)

Comment 89

[Randell Jesup [:jesup] (needinfo me)]

I looked at ESR24.  The patch has no chance of applying there; even the monitors we're using don't exist.  Given the large difference and lifetime of ESR24, padenot and I both suggest WONTFIX

Comment 90

[Justin Wood (:Callek)]

Fixed in SeaMonkey 2.26.1 (Gecko 29 based)

https://hg.mozilla.org/releases/mozilla-release/rev/2d191d69f740

Comment 91

[Matt Wobensmith [:mwobensmith][:matt:]]

I ran the tests from comment 61 many times, but still no crash for me. I've never been able to reproduce this locally, so I'm marking [qa-] for posterity.