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

Status: RESOLVED DUPLICATE of bug 956338

(Core :: XPCOM, defect)

Description

[Honza Bambas (:mayhemer)]

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

[(no longer active)]

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.

Comment 2

[Andrei Purice]

Hey Honza,
Does this issue still reproduce for you or can we close it?

Comment 3

[Andrew McCreight [:mccr8]]

This is still an issue. These classes continue to not be threadsafe.

Comment 4

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

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 5

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #3)

This is still an issue. These classes continue to not be threadsafe.

It looks as if we did at least something in bug 956338 ? Can we restate the remaining scope here?

Comment 6

[Andrew McCreight [:mccr8]]

Yeah, that looks sufficient to me. Dynamic checking isn't a perfect audit, but it seems close to me.