Closed Bug 333669 Opened 14 years ago Closed 10 years ago

Dereferencing null dispData in nsEventListenerManager::HandleEvent

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED INVALID

People

(Reporter: jwatt, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, crash)

Attachments

(1 file, 1 obsolete file)

If we have a NS_USER_DEFINED_EVENT we can end up dereferencing null pointer dispData.
Bryner, do you know what this was supposed to do in this case?
Flags: blocking1.9a1?
I think DispatchToInterface isn't ever called when
message == NS_USER_DEFINED_EVENT.
HandleEventSubType is used for those events.
If that's the case, we should assert so at the beginning of the method.
Marking wanted-1.9 for someone to figure out if it's a false positive.
Flags: blocking1.9a1? → blocking1.9-
Whiteboard: [wanted-1.9]
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.
Here's my stacktrace for this.
Taking over...
Status: NEW → ASSIGNED
Assignee: events → ehsan.akhgari
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch Simple fix (obsolete) — Splinter Review
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.
Attachment #273223 - Flags: superreview?
Attachment #273223 - Flags: review?
Attachment #273223 - Flags: superreview?(jst)
Attachment #273223 - Flags: superreview?
Attachment #273223 - Flags: review?(bzbarsky)
Attachment #273223 - Flags: review?
Comment on attachment 273223 [details] [diff] [review]
Simple fix

I'm not the right reviewer for this.
Attachment #273223 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
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 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.
Attachment #273223 - Flags: superreview?(jst)
Attachment #273223 - Flags: review?(Olli.Pettay)
Attachment #273223 - Flags: review-
(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.
EVENT_TYPE_DATA_EQUALS has null checks, so useTypeInterface should 
be false whenever typeData is null.
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?
Attachment #273223 - Attachment is obsolete: true
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...
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
QA Contact: ian → events
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Yes, sometimes when callers give you bad arguments, crashing is the right thing to do.  You can fix the caller, right?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.