333669 - Dereferencing null dispData in nsEventListenerManager::HandleEvent

Status: RESOLVED INVALID

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jonathan Watt [:jwatt]]

If we have a NS_USER_DEFINED_EVENT we can end up dereferencing null pointer dispData.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bryner, do you know what this was supposed to do in this case?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I think DispatchToInterface isn't ever called when
message == NS_USER_DEFINED_EVENT.
HandleEventSubType is used for those events.

Comment 3

[Boris Zbarsky [:bzbarsky]]

If that's the case, we should assert so at the beginning of the method.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Marking wanted-1.9 for someone to figure out if it's a false positive.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

It is a false positive because typeData is also null so DispatchToInterface won't be called.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventListenerManager.cpp&rev=1.229&mark=1682,1683,1685,1727,1729#1682

Comment 6

[Robert Kaiser]

From what I see, the lines linked by smaug above are now http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventListenerManager.cpp&rev=1.277&mark=1142,1207#1142 and there's no check for what he was talking about, so all the rework done there might have exactly this crash come to the surface now. I have a stacktrace for crashing exactly in that function call, I'll attach it shortly.

Comment 7

[Robert Kaiser]

Attachment: stack trace for crash in DispatchToInterface

Here's my stacktrace for this.

Comment 8

[(no longer active)]

Taking over...

Comment 9

[(no longer active)]

Attachment: Simple fix

The patch checks both |typeData| and |dispData|.  Even though currently either they're both null or they're both non-null, checking for both makes sure no regressions would occur in future.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273223 [details] [diff] [review]
Simple fix

I'm not the right reviewer for this.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 273223 [details] [diff] [review]
Simple fix

So what would this fix?
As you said, "they're both null or they're both non-null"
The crash KaiRo saw could be something else. Badly implemented eventhandler?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 273223 [details] [diff] [review]
Simple fix

And if they both are not null, that is something we'd might want to assert but not have checks in opt-builds, imo, because that means the bug is somewhere else.

Comment 13

[(no longer active)]

(In reply to comment #11)
> (From update of attachment 273223 [details] [diff] [review])
> So what would this fix?
> As you said, "they're both null or they're both non-null"

When they're both null, the |DispatchToInterface| call tries to dereference both through |dispData->method| and |typeData->iid|, which could lead to a crash.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

EVENT_TYPE_DATA_EQUALS has null checks, so useTypeInterface should 
be false whenever typeData is null.

Comment 15

[(no longer active)]

Comment on attachment 273223 [details] [diff] [review]
Simple fix

(In reply to comment #14)
> EVENT_TYPE_DATA_EQUALS has null checks, so useTypeInterface should 
> be false whenever typeData is null.
> 

Hmmm that's right.  So, should we close this bug as INVALID?

Comment 16

[Robert Kaiser]

So the |if (typeData)| referenced in comment #5 in the old code was useless and I did "correctly" crash? It's OK if I did lose 50 lines of typed test in an HTML form when a keypress event crashed, just because you tell me I can't crash there?
Sounds somehow strange to me. I thought it's better to not crash in any case, even if a function gets called with "wrong" parameters...

Comment 17

[Jesse Ruderman]

Yes, sometimes when callers give you bad arguments, crashing is the right thing to do.  You can fix the caller, right?