1219339 - (CVE-2016-1973) Race condition in GetStaticInstance can cause use after free

Status: RESOLVED FIXED

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

Description

[q1]

GetStaticInstance (media\webrtc\trunk\webrtc\system_wrappers\interface\static_instance.h) can experience a race condition. If this occurs, it spuriously decrements its reference count, causing the object protected by it to be destroyed and its memory to be released before all of the code streams using it have been terminated.


Details
-------

The bug is in the handling of the kAddRef case (which is used in at least 2 places in the codebase [1]), and is illustrated by a thread race graph below the affected code:

34: template <class T>
35: // Construct On First Use idiom. Avoids
36: // "static initialization order fiasco".
37: static T* GetStaticInstance(CountOperation count_operation) {
38:   // TODO (hellner): use atomic wrapper instead.
39:   static volatile long instance_count = 0;
40:   static T* volatile instance = NULL;
41:   CreateOperation state = kInstanceExists;
42: #ifndef _WIN32
...
89: #else  // _WIN32
...
107:   } else if (count_operation == kAddRef) {
108:     if (instance_count == 0) {
109:       state = kCreate;
110:     } else {
111:       if (1 == InterlockedIncrement(&instance_count)) {
112:         // InterlockedDecrement because reference count should not be
113:         // updated just yet (that's done when the instance is created).
114:         InterlockedDecrement(&instance_count);
115:         state = kCreate;
116:       }
117:     }
118:   } else { [kRelease]
119:     int new_value = InterlockedDecrement(&instance_count);
120:     if (new_value == 0) {
121:       state = kDestroy;
122:     }
123:   }
124: 
125:   if (state == kCreate) {
126:     // Create instance and let whichever thread finishes first assign its
127:     // local copy to the global instance. All other threads reclaim their
128:     // local copy.
129:     T* new_instance = T::CreateInstance();
130:     if (1 == InterlockedIncrement(&instance_count)) {
131:       InterlockedExchangePointer(reinterpret_cast<void * volatile*>(&instance),
132:                                  new_instance);
133:     } else {
134:       InterlockedDecrement(&instance_count);
135:       if (new_instance) {
136:         delete static_cast<T*>(new_instance);
137:       }
138:     }
139:   } else if (state == kDestroy) {
140:     T* old_value = static_cast<T*>(InterlockedExchangePointer(
141:         reinterpret_cast<void * volatile*>(&instance), NULL));
142:     if (old_value) {
143:       delete static_cast<T*>(old_value);
144:     }
145:     return NULL;
146:   }
147: #endif  // #ifndef _WIN32
148:   return instance;
149: }


Assume |instance_count == 0| and |instance == nullptr| initially, and |count_operation == kAddRef| for both threads:

Thread 1                                       Thread 2
--------                                       --------

Execute lines 107-09, notice that              Execute lines 107-09, notice that     
|instance_count == 0| and thus set             |instance_count == 0| and thus set
|state = kCreate|                              |state = kCreate|

Execute lines 125-29, notice that              Execute lines 125-29, notice that     
|state == kCreate| and create a                |state == kCreate| and create a
new instance of T into |*new_instance|         new instance of T into |*new_instance|

Execute line 130, notice that
|instance_count == 1|. Execute lines 131-32,
replacing |instance| with |new_instance|,
then execute line 148,
returning |instance| to the caller.

                                               Execute line 130, notice that |instance_count == 2|,
                                               and execute lines 134-37, decrementing |instance_count| to 1,
                                               and deleting |new_instance|, then execute line 148,
                                               returning |instance| to the caller.

Use |instance| for awhile...                   Use |instance| for awhile...

Call GetStaticInstance with |kRelease|.
Execute lines 119-21, decrementing
|instance_count| to 0, noticing that,
and setting |state = kDestroy|.
Execute lines 140-41 (nulling |instance|)
and lines 142-43 (destroying and freeing
|instance|'s old value.

                                               Continue using the now-destroyed and freed |instance|.


BTW, I am also unsure whether the ordinary reads of |instance_count| and |instance| are compatible with the interlocked operations performed on those variables under all relevant architectures.

This bug is still present in the current trunk: http://hg.mozilla.org/mozilla-central/file/fc706d376f06/media/webrtc/trunk/webrtc/system_wrappers/interface/static_instance.h .

[1] media\webrtc\trunk\webrtc\modules\rtc_rtcp\source\ssrc_database.cc and media\webrtc\trunk\webrtc\system_wrappers\source\trace_impl.cc .
    It is also used in the test code media\webrtc\trunk\webrtc\test\channel_transport\udp_socket_manager_wrapper.cc .

Comment 1

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

Attachment: proposed rewrite of GetStaticInstance

Rewritten from scratch using Mozilla (c++11) Atomics.  Tries very hard to keep the normal paths away from the critical section via the Atomics.  Note that the common usage (especially for TraceImpl) uses pairs of kAddRefNoCreate/kRelease, with kAddRef being used ~once when logging is enabled for each call.

Comment 2

[q1]

(In reply to Randell Jesup [:jesup] from comment #1)
> Created attachment 8683486 [details] [diff] [review]
> proposed rewrite of GetStaticInstance
> 
> Rewritten from scratch using Mozilla (c++11) Atomics.  Tries very hard to
> keep the normal paths away from the critical section via the Atomics.  Note
> that the common usage (especially for TraceImpl) uses pairs of
> kAddRefNoCreate/kRelease, with kAddRef being used ~once when logging is
> enabled for each call.

This seems more complex than necessary. Why not just do everything under the critical section, so everyone easily can see that it's correct? Is this code used in places where the performance impact justifies the complexity?

Comment 3

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

(In reply to q1 from comment #2)
> (In reply to Randell Jesup [:jesup] from comment #1)
> > Created attachment 8683486 [details] [diff] [review]
> > proposed rewrite of GetStaticInstance
> > 
> > Rewritten from scratch using Mozilla (c++11) Atomics.  Tries very hard to
> > keep the normal paths away from the critical section via the Atomics.  Note
> > that the common usage (especially for TraceImpl) uses pairs of
> > kAddRefNoCreate/kRelease, with kAddRef being used ~once when logging is
> > enabled for each call.
> 
> This seems more complex than necessary. Why not just do everything under the
> critical section, so everyone easily can see that it's correct? Is this code
> used in places where the performance impact justifies the complexity?

Yes, this is used on a hot-path heavily-contended logging function (WebRTC TraceImpl) in realtime code.  The other usage (SSRC database) isn't perf-critical.  I'd use something very like a simple "static T* GetStaticInstance() { static T instance; return &instance; }  IF TraceImpl wasn't big (megabytes when in use), because that would cause it to linger.

I wanted to take a run at an Atomic version of this; there are other ways to do it, but most involve a mutex somewhere.   We can try alternates as well, and a coming update of webrtc.org code gets rid of most of the buffers/size for TraceImpl, which would allow us to keep it around.  (MB of unused ram after one webrtc use will cause problems for mobile).

Comment 4

[q1]

I think there's a bug in the patch.

Imagine that thread 1, handling a kRelease, decrements the refcnt to 0, then executes the |if| below and gets preempted before executing the .exchange:

      if (instance_count == 0) {
        delete_me = instance.exchange(NULL); // XXX is exchange needed?
      }

Now imagine that thread 2 does a kAddRef, bumping the refcnt to 1 and then getting preempted.

And then imagine that thread 3 does a kAddRef, bumping the refcnt to 2 and then executing:

    if (old_count > 0) {
      // don't need to create it
      T* value = instance;
      if (value) {
        return value;
      }

So thread 3 gets |instance|.

Now thread 1 executes, does the exchange, and deletes |instance|. Ick.

Comment 5

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

(In reply to q1 from comment #4)
> I think there's a bug in the patch.

> Now thread 1 executes, does the exchange, and deletes |instance|. Ick.

Yeah, that's why this sort of thing is "fun"....  We may need a post-exchange check of the count before doing anything with it.  And if so we need to be careful about the type of Atomic constraints used, and may need to change from ReleaseAcquire.

I'll probably put up a pure critical-section-based patch as an alternative.  There is some concern about use of that in the field, due to perf.  As a high-performance alternative, I'll also put up a c++11 static-constructor based patch, and see if I can find a way around the memory hit (perhaps some way to drop the data and let it by dynamically re-allocated if needed).

Comment 6

[q1]

(In reply to Randell Jesup [:jesup] from comment #5)
> (In reply to q1 from comment #4)
> > I think there's a bug in the patch.
> 
> > Now thread 1 executes, does the exchange, and deletes |instance|. Ick.
> 
> Yeah, that's why this sort of thing is "fun"....  We may need a
> post-exchange check of the count before doing anything with it.  And if so
> we need to be careful about the type of Atomic constraints used, and may
> need to change from ReleaseAcquire.

There are some simple contention-reduction approaches using eventcounts and spinlocks, such as http://www.cs.uml.edu/~bill/cs515/Eventcounts_Sequencers_Reed_Kanodia_79.pdf .

> I'll probably put up a pure critical-section-based patch as an alternative. 
> There is some concern about use of that in the field, due to perf.  

It'd be interesting to see what impact that has on performance.

> As a
> high-performance alternative, I'll also put up a c++11 static-constructor
> based patch, and see if I can find a way around the memory hit (perhaps some
> way to drop the data and let it by dynamically re-allocated if needed).

How much memory does it use? Unless it's huge, perhaps it doesn't much matter because the OS eventually pushes unused memory out of the working set, meaning that really all it consumes is virtual address space.

Comment 7

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

(In reply to q1 from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #5)
> > (In reply to q1 from comment #4)
> > > I think there's a bug in the patch.
> > 
> > > Now thread 1 executes, does the exchange, and deletes |instance|. Ick.
> > 
> > Yeah, that's why this sort of thing is "fun"....  We may need a
> > post-exchange check of the count before doing anything with it.  And if so
> > we need to be careful about the type of Atomic constraints used, and may
> > need to change from ReleaseAcquire.
> 
> There are some simple contention-reduction approaches using eventcounts and
> spinlocks, such as
> http://www.cs.uml.edu/~bill/cs515/Eventcounts_Sequencers_Reed_Kanodia_79.pdf

Perhaps...  Spinlocks are very bad in most realtime code.  And realtime code has deadlines; statistically taking a reasonable amount of time isn't good if it occasionally takes much longer.

> > I'll probably put up a pure critical-section-based patch as an alternative. 
> > There is some concern about use of that in the field, due to perf.  
> 
> It'd be interesting to see what impact that has on performance.

Yes.  And it might be ok-ish, though on the edges of performance it may suffer.

> > As a
> > high-performance alternative, I'll also put up a c++11 static-constructor
> > based patch, and see if I can find a way around the memory hit (perhaps some
> > way to drop the data and let it by dynamically re-allocated if needed).
> 
> How much memory does it use? Unless it's huge, perhaps it doesn't much
> matter because the OS eventually pushes unused memory out of the working
> set, meaning that really all it consumes is virtual address space.

one word: mobile.  And this is multi-megabytes; on b2g (firefoxos) this is a bug chunk of ram and no device to page it to.

Comment 8

[q1]

(In reply to Randell Jesup [:jesup] from comment #7)
> (In reply to q1 from comment #6)
> > (In reply to Randell Jesup [:jesup] from comment #5)
> > > (In reply to q1 from comment #4)
> > > > I think there's a bug in the patch.
> > > 
> > > > Now thread 1 executes, does the exchange, and deletes |instance|. Ick.
> > > 
> > > Yeah, that's why this sort of thing is "fun"....  We may need a
> > > post-exchange check of the count before doing anything with it.  And if so
> > > we need to be careful about the type of Atomic constraints used, and may
> > > need to change from ReleaseAcquire.
> > 
> > There are some simple contention-reduction approaches using eventcounts and
> > spinlocks, such as
> > http://www.cs.uml.edu/~bill/cs515/Eventcounts_Sequencers_Reed_Kanodia_79.pdf
> 
> Perhaps...  Spinlocks are very bad in most realtime code.  

I disagree. They are the fastest way to implement critical sections. You just do:

   if (InterlockedTestAndSet (&lockbit)) { } // Wait while someone else is in critical section
   // You're in the critical section, do your thing.
   InterlockedClear (&lockbit); // Exit critical section

Within a few instructions' time of the owning thread exiting the critical section, a waiting thread enters it.

The other typical way of doing this involves calls into the OS scheduler (via mutexes, semaphores, and the like), which typically is much slower.

I'd also note that spinlocks are generally the only available multiprocessor synchronization mechanism for code executing in device drivers at interrupt (high) hardware priorities. (Of course in such applications you do the rock-bottom minimum while under a spinlock so that you don't delay other device drivers or the OS.)

The chief drawback of spinlocks is not lack of speed, but waste of CPU cycles when there is contention. You can obviate this problem (if required -- it might not be) by using sleep() and similar approaches, at the cost of increased wake latency. In a usermode environment like FF you can also combine spinlocks with scheduler primitives like Windows events. Another drawback is that plain spinlocks don't guarantee FIFO service to waiting threads, but I suspect that wouldn't be an issue in a soft realtime environment like FF.

> And realtime code
> has deadlines; statistically taking a reasonable amount of time isn't good
> if it occasionally takes much longer.

Agreed.

> > > I'll probably put up a pure critical-section-based patch as an alternative. 
> > > There is some concern about use of that in the field, due to perf.  
> > 
> > It'd be interesting to see what impact that has on performance.
> 
> Yes.  And it might be ok-ish, though on the edges of performance it may
> suffer.

Is there a good way to time it?
 
> > > As a
> > > high-performance alternative, I'll also put up a c++11 static-constructor
> > > based patch, and see if I can find a way around the memory hit (perhaps some
> > > way to drop the data and let it by dynamically re-allocated if needed).
> > 
> > How much memory does it use? Unless it's huge, perhaps it doesn't much
> > matter because the OS eventually pushes unused memory out of the working
> > set, meaning that really all it consumes is virtual address space.
> 
> one word: mobile.  And this is multi-megabytes; on b2g (firefoxos) this is a
> bug chunk of ram and no device to page it to.

Excellent point. I totally missed that issue.

Comment 9

[q1]

Ack!

>    if (InterlockedTestAndSet (&lockbit)) { } // Wait while someone else is in critical section

should of course be

   while (InterlockedTestAndSet (&lockbit)) { } // Wait while someone else is in critical section

Comment 10

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

Bug 1198458 removed the memory hit, so a patch like
"static T* GetStaticInstance() { static T instance; return &instance; }"
no longer has the negative impact.  I'll put that up.

Comment 11

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

Attachment: Greatly simplify GetStaticInstance since we don't allocate MB of buffers now

Need the ifdef for windows/posix because TraceImpl is abstract, btw

Comment 12

[q1]

(In reply to Randell Jesup [:jesup] from comment #10)
> Bug 1198458 removed the memory hit, so a patch like
> "static T* GetStaticInstance() { static T instance; return &instance; }"
> no longer has the negative impact.  I'll put that up.

That also has a race in non-C++11-compliant compilers; see https://bugzilla.mozilla.org/show_bug.cgi?id=1230768 . You have to use a module-level static, but even then it's important to beware order-of-initialization bugs; see C++11 s.3.6.2(2).

Comment 13

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

We require c++11 compilers for mozilla's code.  MSVC 2013 is the lowest level supported; we use this pattern elsewhere as well.

Comment 14

[q1]

(In reply to Randell Jesup [:jesup] from comment #13)
> We require c++11 compilers for mozilla's code.  MSVC 2013 is the lowest
> level supported; we use this pattern elsewhere as well.

VS 2013 also generates thread-unsafe code for the return-address-of-local-static pattern. I just tested it.

Comment 15

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

(In reply to q1 from comment #14)
> (In reply to Randell Jesup [:jesup] from comment #13)
> > We require c++11 compilers for mozilla's code.  MSVC 2013 is the lowest
> > level supported; we use this pattern elsewhere as well.
> 
> VS 2013 also generates thread-unsafe code for the
> return-address-of-local-static pattern. I just tested it.

I'm guessing that's bug 1230768?  That could be problematic, depending on what how it goes wrong.  A Module-level static doesn't help us with deferring creation to first use, note.

Comment 16

[q1]

(In reply to Randell Jesup [:jesup] from comment #15)
> (In reply to q1 from comment #14)
> > (In reply to Randell Jesup [:jesup] from comment #13)
> > > We require c++11 compilers for mozilla's code.  MSVC 2013 is the lowest
> > > level supported; we use this pattern elsewhere as well.
> > 
> > VS 2013 also generates thread-unsafe code for the
> > return-address-of-local-static pattern. I just tested it.
> 
> I'm guessing that's bug 1230768?  

Yes.

> That could be problematic, depending on
> what how it goes wrong.

Ya. The pattern is used widely, but I don't know the codebase nearly well enough to say which usages might be subject to races.

> A Module-level static doesn't help us with
> deferring creation to first use, note.

True. You need some kind of thread-safe template singleton class implemented with a class-level static that's accessed with interlocked instructions, and that allows lazy creation. There's something like this in security\sandbox\chromium\base\memory\singleton.h/.cc , but I haven't reviewed it for correctness.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8696261 [details] [diff] [review]
Greatly simplify GetStaticInstance since we don't allocate MB of buffers now

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

The static variable here is subject to thread races, so I don't think this is going to fly.  Can we really not do double-checked locking here?  That would at least only lock the mutex once.

Comment 18

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

Attachment: switch GetStaticInstance to use IPC's Singleton<T> impl (WIP)

doesn't build for the unittests... darn linkage visibility issues

Comment 19

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

Comment on attachment 8702385 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl (WIP)

So here's a proposed patch (and template for dealing with singletons elsewhere and avoid the static T instance; return &instance; pattern that's broken in MSVC 2013).

The one downside is that this doesn't build for the unittests (I tried to work around the problem, but failed due to pain in fighting the compiler/build system).

The errors I get for all the unit tests look like this:

 0:11.50 ../../../../../../../ipc/chromium/src/base/atomicops_internals_x86_gcc.h:204: error: undefined reference to 'AtomicOps_Internalx86CPUFeatures'
 0:11.50 ../../../../../../../ipc/chromium/src/base/singleton.h:134: error: undefined reference to 'base::AtExitManager::RegisterCallback(void (*)(void*), void*)'
 0:11.50 ../../../../../../../ipc/chromium/src/base/singleton.h:150: error: undefined reference to 'PlatformThread::YieldCurrentThread()'
 0:11.50 ../../../../../../../ipc/chromium/src/base/singleton.h:134: error: undefined reference to 'base::AtExitManager::RegisterCallback(void (*)(void*), void*)'
 0:11.50 ../../../../../../../ipc/chromium/src/base/singleton.h:150: error: undefined reference to 'PlatformThread::YieldCurrentThread()'

Basically, it can't access the ipc objects from libxul (sigh).

Note that the attempt I made to sidestep this in the patch (using FakeIPC.h) fails; they still are undefined (not sure why).

I could use suggestions on how to resolve this without too much pain.  Thanks!

Comment 20

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

Attachment: switch GetStaticInstance to use IPC's Singleton<T> impl (WIP)

Ok, solved the linkage problem by splitting the impl from teh definition (strange, but ok).  Will run a try and see if we need to do something like I did to stub the atomicops stuff on Linux

Comment 21

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

Attachment: switch GetStaticInstance to use IPC's Singleton<T> impl

Comment 22

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

q1 - you may want to check singleton.h's impl before we land.  Thanks!

Comment 23

[q1]

(In reply to Randell Jesup [:jesup] from comment #22)
> q1 - you may want to check singleton.h's impl before we land.  Thanks!

I will try to do this review over the next few days.

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 8702916 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

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

Apologies for the delay.  singleton.h looks reasonable.  r=me with satisfactory answers to the below questions.

::: media/webrtc/signaling/test/FakeIPC.cpp
@@ +4,5 @@
> +
> +#include "FakeIPC.h"
> +#include <unistd.h>
> +
> +// The implementations can't be in the .h file for some annoying reason

Is this comment still true, or is it a remnant of working things out on try?

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/ssrc_database.h
@@ +32,4 @@
>      SSRCDatabase();
>      virtual ~SSRCDatabase();
>  
> +protected:

What is this change for?

Comment 25

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

> ::: media/webrtc/signaling/test/FakeIPC.cpp
> @@ +4,5 @@
> > +
> > +#include "FakeIPC.h"
> > +#include <unistd.h>
> > +
> > +// The implementations can't be in the .h file for some annoying reason
> 
> Is this comment still true, or is it a remnant of working things out on try?

It's true.  C++ sucks sometimes.

> 
> ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/ssrc_database.h
> @@ +32,4 @@
> >      SSRCDatabase();
> >      virtual ~SSRCDatabase();
> >  
> > +protected:
> 
> What is this change for?

This has to be exposed due using the revised GetStaticInstance with SSRCDatabase.  This is without that change:

 0:21.77 ../../../../../../../ipc/chromium/src/base/singleton.h: In instantiation of ‘static Type* DefaultSingletonTraits<Type>::New() [with Type = webrtc::SSRCDatabase]’:
 0:21.77 ../../../../../../../ipc/chromium/src/base/singleton.h:129:33:   required from ‘static Type* Singleton<Type, Traits, DifferentiatingType>::get() [with Type = webrtc::SSRCDatabase; Traits = DefaultSingletonTraits<webrtc::SSRCDatabase>; DifferentiatingType = webrtc::SSRCDatabase]’
 0:21.77 ../../../../../../../media/webrtc/trunk/webrtc/system_wrappers/interface/static_instance.h:39:27:   required from ‘T* webrtc::GetStaticInstance(webrtc::CountOperation) [with T = webrtc::SSRCDatabase]’
 0:21.78 /home/jesup/src/mozilla/inbound_prof/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/ssrc_database.cc:37:54:   required from here
 0:21.78 /home/jesup/src/mozilla/inbound_prof/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/ssrc_database.cc:85:1: error: ‘webrtc::SSRCDatabase::SSRCDatabase()’ is protected

Comment 26

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

Comment on attachment 8702916 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very tough.  Provoking a thread-race in GetStaticInstance would not be easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really; though one could infer (knowing it landed on a hidden bug) that there's a race there somewhere.

Which older supported branches are affected by this flaw?
All.  

If not all supported branches, which bug introduced the flaw?
N/A - upstream bug

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Trivial backport.  Need to double-check that we don't waste memory for MB of trace buffers in the before-webrtc.org-43 revs (FF44 and earlier); I think we're good with singleton.h.  (c++11 static getInstance would waste memory in 44 and earlier).

How likely is this patch to cause regressions; how much testing does it need?  Unlikely to cause regressions; uses a widely-known singleton class and remove a bunch of custom code for doing that.

Comment 27

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

sec-approval+ for trunk. 

Shouldn't we be taking this on ESR38 as well?

Comment 28

[q1]

(In reply to q1 from comment #23)
> (In reply to Randell Jesup [:jesup] from comment #22)
> > q1 - you may want to check singleton.h's impl before we land.  Thanks!
> 
> I will try to do this review over the next few days.

I'm sorry that I never did this review, but I plead excuse because there has been a medical emergency in my family. I will be pretty spotty on bugs for awhile, alas, but I will eventually be back at it.

Comment 29

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

Attachment: switch GetStaticInstance to use IPC's Singleton<T> impl

per the comment, recursing in the singleton creation deadlocks, so don't do that.  Note that the code that logged during TraceImpl creation is windows-only.

Comment 30

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

(In reply to q1 from comment #28)
> (In reply to q1 from comment #23)
> > (In reply to Randell Jesup [:jesup] from comment #22)
> > > q1 - you may want to check singleton.h's impl before we land.  Thanks!
> > 
> > I will try to do this review over the next few days.
> 
> I'm sorry that I never did this review, but I plead excuse because there has
> been a medical emergency in my family. I will be pretty spotty on bugs for
> awhile, alas, but I will eventually be back at it.

Not in any way a problem; I hope everyone is ok.

I updated the patch because there was a windows-only deadlock (recursing trying to Log while instantiating the Log object).

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 8707593 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

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

The deadlocks from the commented-out logging statements were 100% reproducible?  i.e. we shouldn't have to worry about other random deadlocks?

Comment 32

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

100% reproducible, yes.  I was double-checking before landing and found windows wouldn't work at all.

Comment 33

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

Carrying forward approvals...
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb19b60d24a

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out for WinXP crashtest permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=19807692&repo=mozilla-inbound

https://hg.mozilla.org/mozilla-central/rev/878033e3ca98

Comment 35

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

Attachment: switch GetStaticInstance to use IPC's Singleton<T> impl

On WinXP (only) it apparently hit a second set of WEBRTC_TRACE() macros while instantiating the TraceWindows object; they're commented out now.  I audited system_wrappers for any further trace calls it could hit, and there aren't any more.  (just 2 remain, in the posix impl of SetThreadPriority()).

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 8708241 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

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

This is one of those patches where it would have been super-nice to have automatic interdiffs from MozReview...

Comment 37

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6653283565a0
(I forced winXP tests this time - previous landing I hadn't forced them in Try)

Comment 38

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

Also I tested manually on a WinXP tester box remotely.

Comment 39

[Daniel Veditz [:dveditz]]

Seems risky to land into the last beta, bumping out to the next release.

Comment 40

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

After discussion with ritu and dveditz, we agreed to land in the next cycle and then uplift to 45.  It's very late in 44 beta.  I have a hopefully better patch ready to review in any case, so that's a plus.  (no risk of deadlocks on recursive GetStaticInstances, for example).

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8702916 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

Clearing the approvals for now so it doesn't show up on the needs-uplift radar.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

No pressure but it is the next release now, if you are (more) ready to land and uplift.

Comment 43

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee641313088

Will uplift after m-c merge

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ee641313088

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Randell, could you fill the uplift requests? Thanks

Comment 46

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

Comment on attachment 8708241 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

Note: already aurora/beta +'d by abillings -- see comment 27; we just had to defer checkin until after 44 had shipped.  I assumed it still held.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very tough.  Provoking a thread-race in GetStaticInstance would not be easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really; though one could infer (knowing it landed on a hidden bug) that there's a race there somewhere.

Which older supported branches are affected by this flaw?
All.  

If not all supported branches, which bug introduced the flaw?
N/A - upstream bug

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Trivial backport.  Need to double-check that we don't waste memory for MB of trace buffers in the before-webrtc.org-43 revs (FF44 and earlier); I think we're good with singleton.h.  (c++11 static getInstance would waste memory in 44 and earlier).

How likely is this patch to cause regressions; how much testing does it need?  Unlikely to cause regressions; uses a widely-known singleton class and remove a bunch of custom code for doing that.

Comment 47

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

Note: I may want to hold off for a few hours or a day on the uplift while we check if a minor tweak is needed when using Webrtc internal logging in debug builds.

Comment 48

[Paul Kerr [:pkerr]]

Attachment: Part2: Ensure close of webrtc trace file during shutdown

Comment 49

[Paul Kerr [:pkerr]]

Comment on attachment 8717723 [details] [diff] [review]
Part2: Ensure close of webrtc trace file during shutdown

Created a StopWebRtcLog() call that closes any trace file opened either from the environment vars or using about:webrtc. I moved the EnableWebRtcLog() calls from VideoConduit.cpp and AudioConduit.cpp into the global init of PeerConnectionCtx. This makes the setup of logging from environment vars happen at the global singleton level in both MediaManager (where it already existed) and PeerConnection. Both these modules now call StopWebRtcLog() when shutting down. (Putting a StopWebRtcLog() call in VideoConduit.cpp or AudioConduit.cpp would have closed the log whenever a single session ended.)

Comment 50

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

Comment on attachment 8717723 [details] [diff] [review]
Part2: Ensure close of webrtc trace file during shutdown

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

r- because of loss of logs from plain getUserMedia.  simply don't change AudioConduit of VideoConduit, and that should be r+, though try it!

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ -256,5 @@
>      CSFLogError(logTag, "%s Unable to create voice engine", __FUNCTION__);
>      return kMediaConduitSessionNotInited;
>    }
>  
> -  EnableWebRtcLog();

Removing these means we can't collect logs for getUserMedia (without PeerConnection).  that's doable, but not preferable.  I think we can leave these in, as MediaManager will shut it down in an observer.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -307,5 @@
>        }
>      }
>    }
>  
> -  EnableWebRtcLog();

ditto

Comment 51

[Paul Kerr [:pkerr]]

There is already a call to EnableWebRtcLog() in MediaManager.cpp. It is called during the construction that is trigger by the first call to GetUserMedia. I have tested this with the GUM test and the log is created.

However, putting the enables back into the AudioConduit and VideoConduit will not hurt anything. I will go ahead if you feel it is needed.

Comment 52

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

So long as the logs are getting enabled for normal gUM, I'm good with it.  r+

Comment 53

[Paul Kerr [:pkerr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e15ecccb6c

Comment 54

[Liz Henry (:lizzard) (relman/hg->git project)]

Waiting for this to land on m-c before uplift to aurora and beta.

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5e15ecccb6c

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Randell, Paul, could you please nominate the patches for uplift to esr38? Thanks!

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8708241 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

Sec-high, taking it.
Should be in 45 beta 7

Comment 58

[Paul Kerr [:pkerr]]

Comment on attachment 8717723 [details] [diff] [review]
Part2: Ensure close of webrtc trace file during shutdown

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: The LateWriteMonitor will be triggered when trace logging is enabled for the webrtc components due to using the singleton class to fix the security bug.
Fix Landed on Version: 47
Risk to taking this patch (and alternatives if risky): Very low risk: added log close calls in the same modules that open the logs.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 59

[Paul Kerr [:pkerr]]

Comment on attachment 8708241 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: This is a sec bug fix to prevent race conditions during shutdown.
Fix Landed on Version: 47
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b3542fbda40
https://hg.mozilla.org/releases/mozilla-beta/rev/92ce85d73acc

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f333c10a8090
https://hg.mozilla.org/releases/mozilla-aurora/rev/20b471d3cfe0

Comment 62

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8708241 [details] [diff] [review]
switch GetStaticInstance to use IPC's Singleton<T> impl

Sec-high that should also be uplifted to ESR38.7

Comment 63

[Carsten Book [:Tomcat]]

part 2 has problems uplifting to esr38 

warning: conflicts while merging dom/media/MediaManager.cpp! (edit, then use 'hg resolve --mark')

Comment 64

[Paul Kerr [:pkerr]]

Taking a look at where to add the StopWebRtcLog() call in the older version. The shutdown logic is different.

Comment 65

[Paul Kerr [:pkerr]]

I have a patch the applies on my clone of the esr38 repo. How do you want to receive it? Attach another file to this bug?

Comment 66

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Paul Kerr [:pkerr] from comment #65)
> I have a patch the applies on my clone of the esr38 repo. How do you want to
> receive it? Attach another file to this bug?

Hi Paul, yes I think that is the right thing to do. Wes, please correct me if I am wrong.

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yeah, attach the new patch here, we'll carry forward the a+ when we push it.

Comment 68

[Paul Kerr [:pkerr]]

Attachment: Part2 (esr38): Ensure close of webrtc trace file during shutdown

ESR38 specific patch. Works with older MediaManager shutdown code.

Comment 69

[Paul Kerr [:pkerr]]

Comment on attachment 8722161 [details] [diff] [review]
Part2 (esr38): Ensure close of webrtc trace file during shutdown

[Approval Request Comment]
Re-implementation of MediaManager portion of patch. See previous esr38 request.

Comment 70

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8722161 [details] [diff] [review]
Part2 (esr38): Ensure close of webrtc trace file during shutdown

Sec-high, taking it in esr38.7

Comment 71

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

remote:   https://hg.mozilla.org/releases/mozilla-esr38/rev/8b0125a79e90
remote:   https://hg.mozilla.org/releases/mozilla-esr38/rev/6037b3d95c6b

Two of the tests part 1 modified don't exist on esr38, so I skipped those particular changes but left the rest of part 1 intact. Jesup says that's the right thing to do, so I hope he's right.

Comment 72

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

(In reply to Wes Kocher (:KWierso) from comment #71)
> Two of the tests part 1 modified don't exist on esr38, so I skipped those
> particular changes but left the rest of part 1 intact. Jesup says that's the
> right thing to do, so I hope he's right.

It is; both of those were added in ~mozilla-45

Comment 73

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out from esr38 because it seems to have caused all of these windows test crashes: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr38&fromchange=b03db72e32f6&filter-searchStr=windows&selectedJob=60687

https://hg.mozilla.org/releases/mozilla-esr38/rev/95ff87488690

Comment 74

[Paul Kerr [:pkerr]]

Will take a look.

Comment 75

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Paul, I will gtb esr38.7 release candidate tomorrow (ideally it should have happened today noon PST). Could you please re-land an updated patch on esr38 branch asap? Thanks!

Comment 76

[Paul Kerr [:pkerr]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/3a606f8182c8

Comment 77

[Paul Kerr [:pkerr]]

Attachment: switch GetStaticInstance to use IPC's Singleton<T> impl

Comment 78

[Paul Kerr [:pkerr]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/9d44c42ca799

Comment 79

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

Paul backed this out for Windows failures (that don't happen locally):
https://hg.mozilla.org/releases/mozilla-esr38/rev/f4220254d5bd

Comment 80

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

IMO, we should not block ESR38 on this patch.  This is a long-standing bug, with minimal actual risk:

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very tough.  Provoking a thread-race in GetStaticInstance would not be easy.

In fact it would be very hard.  Perhaps not impossible, but very hard.

Comment 81

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

[Tracking Requested - why for this release]: Updating status from fixed to affected based on the backout commit in comment 79. Based on :Jesup's comment, I am putting this back in tracking-esr38 queue. We could either wontfix this for esr38.x or reconsider for esr38.8. The latter seems like a fine idea.

Comment 82

[Daniel Veditz [:dveditz]]

ESR 38.8 did seem like a fine idea, but it's too late now.