Closed Bug 1563066 Opened 4 months ago Closed 4 months ago

nsXULPrototypeNode subclasses should have a private dtor

Categories

(Core :: XUL, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

This class is refcounted, but it has a public dtor, which means you can, for instance, make an nsAutoPtr for it. Presumably this indicates a bug in the private dtor for refcounted things static analysis.

Blocks: 1563067
Assignee: nobody → continuation
Group: dom-core-security

Unsurprisingly, this is a problem for more than just the one subclass.

I'm working on a patch, and it indirectly revealed one security bug (so far). edit: It didn't actually. See below.

XULContentSinkImpl::CreateElement() creates a nsXULPrototypeElement, but does not addref it, which is gross, but sort of ok. This function has two call sites. The first, XULContentSinkImpl::OpenTag, calls delete on element if mContextStack.GetTopChildren() fails. The second, XULContentSinkImpl::OpenRoot(), calls Release() on element if a call to mContextStack.Push() fails, which is unsound, because element doesn't have a reference to it! Which is why I hid the bug. But it turns out that XULContentSinkImpl::ContextStack::Push() is actually infallible, so this is not actually a bug.

Group: dom-core-security
Summary: nsXULPrototypeElement should have a private dtor → nsXULPrototypeNode subclasses should have a private dtor
Priority: -- → P2

By adding an optional nodeinfo arg to the nsXULPrototypeElement ctor,
the CreateElement method can be easily inlined.

This lets us remove some error handling code, including some that is
incorrect.

There are a few places that use XUL prototype nodes without ref
pointers, which is dangerous. A benefit of this change is that it
removes a direct call to delete on a refcounted class. It does result
in some additional refcount traffic, but hopefully that doesn't
matter.

Refcounted classes should not ever have delete called on them
directly, because something else might be holding a reference to
them. One way to make this harder is to declare the dtor private.

The immediate motivation for this change is that somebody was using
one of these classes as the value for an nsClassHashtable, which
should be rejected at compile time.

Blocks: 1563320
Blocks: 1563405
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5a0ca3d92cc
part 1 - Inline XULContentSinkImpl::CreateElement. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/98e692885ecc
part 2 - XULContentSinkImpl::ContextStack::Push is infallible. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/fd915382080d
part 3 - Use refptrs in more places in XULContentSinkImpl. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/bfc0d9412df9
part 4 - Make dtors for nsXULPrototypeNode subclasses private. r=bzbarsky
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55e113f30a95
Backed out 4 changesets for build bustages on nsXULElement.h . CLOSED TREE
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aafcf88cbefc
part 1 - Inline XULContentSinkImpl::CreateElement. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/700e7a6894f7
part 2 - XULContentSinkImpl::ContextStack::Push is infallible. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/66bc5ea2691b
part 3 - Use refptrs in more places in XULContentSinkImpl. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/e50e14aed7e4
part 4 - Make dtors for nsXULPrototypeNode subclasses private. r=bzbarsky
You need to log in before you can comment on or make changes to this bug.