508133 - ensure Release() stabilizes

Status: RESOLVED INACTIVE

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(dormant account)]

::Release method implementations should set mRefCnt variable to 1 before "delete this"ing. This apparently helps in cases where the destructor calls methods that may check the ref count.

Comment 1

[Brendan Eich [:brendan]]

The general prob is that some methods may temporarily call AddRef and Release on a reference to the pre-mortem finalizable object -- doing so would of course be disastrous in the call to Release if the ref-count were left at 0 instead of stabilized.

Release is indeed a method that "check[s] the ref count" but it's what the check causes that matters ;-). Asserting non-zero mRefCnt can be painful too, of course.

/be

Comment 2

[Benjamin Smedberg]

In general destructors that require refcount stabilizing indicate bad design somewhere along the way. I don't think we should make a general rule of setting mRefCnt to 1 before destruction... instead asserting that you can't call AddRef or Release after you've started the destruction process.

Comment 3

[Brendan Eich [:brendan]]

I would say it differently: pre-mortem finalization is a problem. We want no chance of resurrecting garbage.

But this is for a different bug. In today's world we have tons of code that must stabilize, because XPCOM supports pre-mortem finalization (destructor called via delete from last Release). That's a fact; I would not call it a design flaw in the consuming code, rather a flaw inherent in XPCOM.

/be

Comment 4

[Benjamin Smedberg]

The client code doesn't matter. Once you've gotten to final-release, the only thing that can resurrect an object is the destructor itself, by somehow passing a ref of the already-dead object out so that it can be resurrected. That's a code error which we should detect and fix now: we don't need defense-in-depth against localized code errors we can just fix.

Comment 5

[Brendan Eich [:brendan]]

My point was that pre-mortem finalization allows arbitrary code to run from a C++ dtor, which sometimes must add and remove temporary refs on |this|. I don't see how you can avoid this in general.

As for defense in depth, we absolutely need it. Any bug where free memory is read by a virtual call instruction sequence should be presumed exploitable.

/be

Comment 6

[Benjamin Smedberg]

A well-designed destructor shouldn't ever need to add temporary refs on |this|, and it's something we can assert and prevent.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> A well-designed destructor shouldn't ever need to add temporary refs on |this|,
> and it's something we can assert and prevent.

Are you calling many of the dtors in dom/base/*.cpp not well-designed?

The refcount stabilizing added long ago was added for a good reason. We shouldn't remove it and hope to test our way out of security bug hell. Without a static proof system of some kind, we absolutely need defense in depth.

/be

Comment 8

[Benjamin Smedberg]

Yes.

The dangers of refcount stabilization (handing out a resurrected reference to an object that is destroyed) has been the source of at least two security-related bugs I've dealt with. If the general rule is that we assert/prevent any ref from being handed out during destruction and make exceptions for the special cases I think we're much better off than making and enforcing a blanket rule that we *should* stabilize the refcount and potentially leak a reference.

Comment 9

[Brendan Eich [:brendan]]

I don't believe you can give a loaded pre-mortem finalization gun to a large audience of hackers and then hope to "test in quality". The design of XPCOM, the layering of last Release on C++ destructor called via delete in particular, makes lack-of-stabilization bugs inevitable. These are exploitable in general, so we must have defense in depth, wherefore stabilization in the boilerplate.

Did you have a static analysis idea to help enforce constraints on |this| flow in destructors?

/be

Comment 10

[Benjamin Smedberg]

A dynamic analysis should be sufficient: once you hit last-release (refcount is 0 and you're firing the destructor), assert if anything tries to addref the object.

You could certainly do a static analysis of 'this' escaping, but I think the annotation burden would be pretty high unless it was a global-flow analysis.

Comment 11

[Brendan Eich [:brendan]]

We have code coverage measures (gcov, whatever)? Dynamic all-paths is a hope at this point. Why remove defense in depth and risk disaster after letting the rules stand for years?

Or did you mean stabilize release builds, assert mRefCnt not 0 in debug?

/be

Comment 12

[Benjamin Smedberg]

I'm not saying we should remove existing stablizers: but I think that the stablization is probably riskier, security-wise, than not stabilizing: you're much more likely to end up with a strong ref to a destroyed object than if you assert/crash...

Comment 13

[Brendan Eich [:brendan]]

Our experience long ago contradicts the thought that stabilization is riskier. I can't find good traces (this was almost before the dawn of mozilla.org, IIRC), but one bug is bug 16105. I think that led waterson to change xpcom/base/nsAgg.h to stabilize. The other CVS blame for stabilization is rev 3.1 dougt (first rev in cvs.mozilla.org open source).

The many FMR -> control flows through forged vtable bugs I've seen over the years do not involve a dangling strong reference taken during last Release. Such a bad pointer (retained after balanced AddRef/Release while the dtor runs) is possible, I've just never seen it. OTOH if you remove stabilization and there is balanced holding and releasing going on, the dtor will reenter. That was the cause of the pain experienced in the ancient days.

/be

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm, I managed to miss this earlier... but I agree that stabilization is bad, adding it in the past was a mistake, and we shouldn't add more of it.