1841706 - StaticRefPtr<> can cause UAFs if used with concurrent threads

Status: NEW

(Core :: XPCOM, defect)

Description

[mozillabugs]

Attachment: ffbug_2168.cpp

StaticRefPtr<> (xpcom/base/StaticPtr.h) typically is used in a manner that, if concurrent threads are in play, could cause UAFs. A typical usage is:

   class someClass {
   ...
      static StaticRefPtr<someClass> gSingleton
      ...
      static someClass * GetSingleton() {
         if (!gSingleton) {
            gSingleton = new someClass(...);
         }
         return gSingleton;
      }
   ...
   };

Now imagine that GetSingleton() is called in 2 threads concurrently, and that both both find the !singleton condition true and execute the new someClass, creating objects o1 and o2. Then:

   thread 1                                        thread 2
   --------                                        --------

   StaticRefPtr::operator=(&o1) {                  StaticRefPtr::operator=(&o2) {
      AssignWithAddref(aRhs);                         AssignWithAddref(aRhs);

   StaticRefPtr::AssignWithAddref(aNewPtr) {       StaticRefPtr::AssignWithAddref(aNewPtr) {
      ...AddRef(aNewPtr);                             ...AddRef(aNewPtr);
      AssignAssumingAddref(aNewPtr);                  AssignAssumingAddref(&aNewPtr);

   StaticRefPtr::AssignAssumingAddref(aNewPtr) {   
      T *oldPtr = mRawPtr; // NULL
      mRawPtr = aNewPtr;   // &o1
      if (oldPtr) { // not taken
      ...
   }

   return gSingleton; // return &o1
   ...
                                                   StaticRefPtr::AssignAssumingAddref(aNewPtr) {
                                                      T *oldPtr = mRawPtr; // &o1
                                                      mRawPtr = aNewPtr;   // &o2
                                                      if (oldPtr) {        // taken
                                                         Release(oldPtr);  // &o1, refcnt is now 0
                                                                           // so it deletes &o1
   GetSingleton() -> oh_oh(); // uses &o1 => UAF!

Attached is a test program (using a simplified version of StaticRefPtr to make it easy to build) that demonstrates the issue. To use it:

1. Build it as a debug-mode console program in VS 2022.
2. Set a BP on the calls to GetSingleton() in thread1f() and thread2f().
3. Run the program.
4. The BP in thread1f() should fire. If not, run the program again.
5. When the BP fires, freeze all but the firing thread (thread 1) and step into the GetSingleton() call. Step to, but not farther than, the new statement. Now freeze this thread, thaw the other threads, and proceed.
6. When the BP in thread2f() fires (in thread 2), step up to, but not including the call to StaticRefPtr::AssignAssumingAddRef(). Now freeze this thread and thaw the other threads.
7. Use the debugger to switch to thread 1 (the one with thread1f() on its stack).
8. Step through the return mpSingleton and back into thread1f(), and then up to, but not into, the call to CheckForPuddyTat(). Record mpSingleton->mRawPtr.
9. Freeze thread 1, thaw the other threads, and use the debugger to switch to thread 2 (the one with thread2f() on its stack).
10. Now step AssignAssumingAddRef() in thread 2. Notice how it calls Release() on the existing mpSingleton->mRawPtr, and then deletes it.
11. Freeze thread 2, thaw the other threads, and switch to thread 1.
12. Examine p. Notice that it contains the address of the object that got deleted in step 10, which is the same address you recorded in step 8.
13. Step into the call to CheckForPuddyTat(). Observe that the test for i != 0 et al succeeds and the program prints a warning.

The issue is that the test and assignment of gSingleton are not atomic. It might take a bit of trickery to make them atomic, to avoid unnecessary constructor calls, and to avoid unintuitive usages. Maybe variadic templates and compare_exchange_strong()?

I will try to find a case in which FF actually can experience this bug.

Comment 1

[Andrew McCreight [:mccr8]]

I think this is a known issue, or at least I was aware of it. Any kind of RefPtr is going to have the same issue. Generally these kinds of static variables are initialized on the main thread from startup, or consistently on some other thread. TSan will yell at us if somebody is doing things wrong, but it is possible we are missing it somewhere.

Comment 2

[mozillabugs]

Attachment: better_atomic_init.cpp

Here's a potential solution to this kind of issue that's pretty clean and easy to use. Comments?

Comment 3

[mozillabugs]

Attachment: better_atomic_init2.cpp

Here's an improved version of better_atomic_init.cpp. Try stepping it in the debugger and freezing/thawing threads as appropriate.

[When I tried submitting this program as inline code, bugzilla said "Couldn’t upload the text as an attachment. Please try again later. Error: Failed to fetch attachment ID 9343816 from S3: The requested key was not found", but it still uploaded it!]

Comment 4

[mozillabugs]

There might be another unfixed bug with TSAN flagging StaticRefPtr: https://bugzilla.mozilla.org/show_bug.cgi?id=1717134 .

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:nika, could you have a look please?

For more information, please visit BugBot documentation.