Closed Bug 798188 Opened 8 years ago Closed 6 years ago

NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY can be a footgun

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: tbsaunde, Assigned: jst)

Details

(Keywords: sec-want)

Attachments

(2 files, 1 obsolete file)

so, I just got bit by this consider you have

NS_INTERFACE_MAP_ENTRY(foo)
NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(nsFoo)

such a QI will not work for getting foo.

NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY is defined roughly as
if (x) {
  return;
}
then NS_DOMINTERFACE_MAP_ENTRY_CLASSINFO is roughly
if (y) {
  stuff
} else
  interfacePtr = 0;

so NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY broke the else if chain, and then  the dom class info else gets everything but the class info iids and nukes any interface that had already been set.
(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> so, I just got bit by this consider you have
> 
> NS_INTERFACE_MAP_ENTRY(foo)
> NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(nsFoo)
> 
> such a QI will not work for getting foo.
> 
> NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY is defined roughly as
> if (x) {
>   return;
> }
> then NS_DOMINTERFACE_MAP_ENTRY_CLASSINFO is roughly
> if (y) {
>   stuff
> } else
>   interfacePtr = 0;
> 
> so NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY broke the else if chain, and then 
> the dom class info else gets everything but the class info iids and nukes
> any interface that had already been set.

actually the foundInterface = 0; part comes from NS_INTERFACE_MAP_END, so just putting the wrapper cache entry last is enough (see bug 840906)
So, I tried to do the obvious thing and add an else to NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY but there's a bunch of dom stuff that does something like this
NS_INTERFACE_MAP_BEGIN(nsFoo)
  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
  NS_INTERFACE_TABLE_4(nsFoo, nsIFoo1, nsIFoo2... )
  NS_DOM_CLASSINFO_INTERFACE_ENTRY
NS_INTERFACE_MAP_END

I could just put the wrapper cache entry after the table in all these cases, but I'm sort of worried that getting the wrapper cache is the common case and putting it after the table stuff will hurt perf some.  We could of course just work around this in some other way (put {} after the wrapper cache entry or something)
Keywords: sec-want
Summary: NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY and NS_DOM_INTERFACE_MAP_ENTRY can combine to be a footgun → NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY can be a footgun
This bit pretty hard today for Mike Habisher in the DOM camera code (see bug 889778, which neither Mike nor I knew about at the time). I propose we add an else to the end of the macro which will eliminate this footgun when used in an interface map. For tables that may or may not work, if it does not, it'll fail with a compiler error, which is a whole lot better than silently causing QI to fail for everything other than wrapper cache when this bites. I'm also proposing we add a table version of this macro (i.e. identical to what we have today) and use that consistently when writing table based QIs.

There may be better ways forward here, and I'm happy to hear about that, but let's not let this keep biting while we figure that out, assuming it's not trivial to solve this in a better way.
Comment on attachment 8477060 [details] [diff] [review]
Add table version of NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY and add else to the end of the existing macro.

Review of attachment 8477060 [details] [diff] [review]:
-----------------------------------------------------------------

We really shouldn't put wrappercache at the end of a QI map. Don't think there's a way to enforce that though :-(.
Attachment #8477060 - Flags: review?(peterv) → review+
Comment on attachment 8477061 [details] [diff] [review]
Use NS_WRAPPERCACHE_INTERFACE_TABLE_ENTRY in table based QIs.

Review of attachment 8477061 [details] [diff] [review]:
-----------------------------------------------------------------

Could you also remove the  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY in SpeechSynthesisUtterance? It inherits from DOMEventTargetHelper which has it already.
Attachment #8477061 - Flags: review?(peterv) → review+
Updated with peterv's suggestion. Carrying forward r+
Attachment #8477061 - Attachment is obsolete: true
Attachment #8478587 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a6946ebd6fed
https://hg.mozilla.org/mozilla-central/rev/8e9ed01b8a50
Assignee: nobody → jst
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.