Investigate doing static analysis on raw pointer member variables which point to some addrefed object.

NEW
Unassigned

Status

()

Core
Rewriting and Analysis
5 years ago
22 days ago

People

(Reporter: smaug, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

There is still some, usually old, code which uses raw pointer to addrefed objects in 
classes. We've had too many security sensitive crashes because of that.
Perhaps we could use static analysis or some such to require either
annotation of such member variables to be ok, or to change them to use nsCOMPtr/nsRefPtr.
Blocks: 430328
Component: General → Rewriting and Analysis

Updated

5 years ago
Group: core-security
I've been thinking about this, too.  The easiest way to look into this would be to remove the implicit conversion from ref counted pointer to raw pointer and see what happens, but I suspect that may have too many false positives to be useful.
(In reply to Andrew McCreight [:mccr8] from comment #1)
> I've been thinking about this, too.  The easiest way to look into this would
> be to remove the implicit conversion from ref counted pointer to raw pointer
> and see what happens, but I suspect that may have too many false positives
> to be useful.

That would break using it as function arguments, which I'm sure make up a very sizable portion of the use of our ref-counted pointers.
Yeah, that's true.

For context, the form that most of these have taken are like this:

nsFoo *x = y->GetWhatever();

Where GetWhatever returns nsRefPtr<nsFoo> (or COMPtr).  If the whatever ends up getting released to 0 while x is still live, badness ensues.

Comment 4

4 years ago
Olli: is this an accurate formulation of what you would like to see here?

Create an analysis which catches any variable declaration of definition of type ({const,volatile})+ T* where T has two (perhaps inherited) members, named "AddRef" and "Release".

I'd like to try to write this analysis.
Flags: needinfo?(bugs)
I don't know what ({const,volatile})+  means in this context, but sure, 
any T which has AddRef and Release, and T* is being used in a non-stack-only class as a member variable. And that class isn't nsCOMPtr<T> nor nsRefPtr<T>.
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.