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

NEW
Unassigned

Status

()

--
critical
5 years ago
a year ago

People

(Reporter: mayhemer, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

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.

Scenario:
- 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:

1: http://hg.mozilla.org/mozilla-central/annotate/325c74addeba/netwerk/base/src/Tickler.h#l56

2: http://hg.mozilla.org/mozilla-central/annotate/325c74addeba/netwerk/cache2/CacheFile.h#l50

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.