Audit use of weak pointers regarding thread-safety (potential use-after-free/double-free)




5 years ago
a year ago


(Reporter: mayhemer, Unassigned)


Firefox Tracking Flags

(Not tracked)




5 years ago
This considers both XPCOM's ns(Supports)WeakReference and mfbt/(Supports)WeakPtr.

There might be a problem when multiple threads access/reference/dereference an object implementing nsSupportsWeakReference or SupportsWeakPtr.

- thread A calls Release() and drops refcnt of an object implementing nsSupportsWeakReference to 0, enters the branch that does |delete this|
- before thread A reaches the |delete this| line thread B takes and addrefs the referent (pointing to the real object) in the related nsWeakReference object ; this may happen because we set the *referent* to null from the real object's dtor that has not yet been reached at this moment
- thread A calls |delete this| on the real object
- thread B works with a deleted object (use-after-free) and when it's Release()'ed it may be (depending on in what state the heap is) double deleted (double-free)

I raise this problem since I've hit this misunderstanding of weak reference usage by two developers recently:



Both demonstrates that refcntrs are implemented as thread-safe (and objects are used/can be dereferenced on any thread) but both implement nsSupportsWeakReference/SupportsWeakPtr despite the above described problem.

If I'm right with the scenario, we must audit the code and fix all places.

Comment 1

5 years ago
I think you're right!

In addition to auditing the existing code, it would be nice if we could do something to fix this and/or protect future callers against it, I just can't think of a way to do either.
You need to log in before you can comment on or make changes to this bug.