309574 - [FIX]Invalid read in nsJSEventListener::HandleEvent

Status: RESOLVED FIXED

Description

[Andrew Schultz]

Following part of the steps frmo bug 309564, valgrind reports an invalid read in
nsJSEventListener::HandleEvent (of freed memory).

Steps to reproduce:
2. Visit the linked URL ( http://demo.planzo.com )
3. Click a box on the calendar and type a comment then press <ENTER>
4. Double-click the event you entered in Step 3.

The page is heavily DHTML and also does AJAX.  I'll attach the info valgrind gives.

Comment 1

[Andrew Schultz]

Created attachment 197000 [details]
valgrind info

Comment 2

[Boris Zbarsky [:bz]]

Andrew, do you still see this?  I can't reproduce...

Comment 3

[Andrew Schultz]

worksforme with trunk although I still see it with a 1.8 branch build.  Is there interest fixing this there?

Comment 4

[Boris Zbarsky [:bz]]

Probably... if we could figure out when this got fixed on trunk (e.g. could we test with nightlies?), that would be a great start.

Comment 5

[Andrew Schultz]

a build from 2005120204 does
a build from 2005120305 does not have the bug

Comment 6

[Boris Zbarsky [:bz]]

Presumably when the JS event listener stuff got changed majorly in bug 241518...

Do we have any idea for the line number or anything like that?  I mean on the branch?

Comment 7

[Andrew Schultz]

Created attachment 218027 [details]
valgrind log for MOZILLA_1_8_BRANCH

line numbers are only a bit off from before

Comment 8

[Boris Zbarsky [:bz]]

Does doing:

  nsCOMPtr<nsIJSEventListener> kungFuDeathGrip(this);

At the top of HandleEvent help?  It looks to me like we die in the event, then try to access our member...

Comment 9

[Andrew Schultz]

yes, the kungFuDeathGrip fixed the valgrind error

Comment 10

[Boris Zbarsky [:bz]]

Created attachment 218492 [details] [diff] [review]
Patch

OK, I see why bug 241518 helped.  This patch should fix the valgrind warning too.

Drivers:  This is a memory read of deleted memory.  I _think_ it's not exploitable (it's just accessing a member variable).  But it might make sense to fix this on the 1.8.0 branch anyway.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 218492 [details] [diff] [review]
Patch

r+sr+a=jst

Comment 12

[Boris Zbarsky [:bz]]

Fixed on 1.8.1 branch.

Comment 13

[Daniel Veditz [:dveditz] PTO until August]

Comment on attachment 218492 [details] [diff] [review]
Patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 14

[Boris Zbarsky [:bz]]

Fixed on 1.8.0 branch.