Last Comment Bug 386382 - 1.8 branch shutdown crash after using accessibility api (testcase from bug 377470)
: 1.8 branch shutdown crash after using accessibility api (testcase from bug 37...
Status: RESOLVED FIXED
[sg:moderate?] critical if unprivileg...
: crash, fixed1.8.1.5, verified1.8.0.13, verified1.8.1.8
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
https://bugzilla.mozilla.org/attachme...
Depends on: 377470
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-29 14:42 PDT by Daniel Veditz [:dveditz]
Modified: 2008-02-08 14:23 PST (History)
4 users (show)
dveditz: wanted1.8.1.x+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack trace (7.82 KB, text/html)
2007-07-03 22:38 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (5.86 KB, patch)
2007-07-03 22:45 PDT, Mats Palmgren (:mats)
aaronlev: review+
Details | Diff | Splinter Review
Patch rev. 2 (6.21 KB, patch)
2007-07-04 10:35 PDT, Mats Palmgren (:mats)
aaronlev: review+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2007-06-29 14:42:24 PDT
I get a shutdown crash in 1.8.1.5pre debug builds after running the testcase from bug 377470 (https://bugzilla.mozilla.org/attachment.cgi?id=261540), stack as described at bug 377470 comment 9, dereferencing a deleted object.

So far have not reproduced the crash in an optimized build.
Comment 1 Mats Palmgren (:mats) 2007-07-03 22:38:27 PDT
Created attachment 270839 [details]
stack trace

nsHTMLComboboxAccessible::Init() creates the button and list accessibles
into its members using GetNextSibling().  The problem is that the
GetNextSibling methods uses mNextSibling to check if it should create
its sibling or not, but doesn't actually assign it, so any subsequent
call to GetNextSibling() will create a new ComboboxListAccessible object.
ComboboxListAccessible caches up option accessibles effectively stealing
the children of the previous one:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp&rev=1.46.2.3&root=/cvsroot&mark=361#345

which now has an invalid mFirstChild pointer - which will eventually
crash when nsAccessible::Shutdown tries to use it.
Comment 2 Mats Palmgren (:mats) 2007-07-03 22:45:24 PDT
Created attachment 270841 [details] [diff] [review]
Patch rev. 1

This should fix it I think.  This code have been redesigned on trunk
(bug 278034) so this looks like a branch-only bug.
Comment 3 Mats Palmgren (:mats) 2007-07-03 22:56:45 PDT
I can reproduce this crash in Firefox 2.0.0.4 on Linux:  TB33713986Z
Comment 4 Aaron Leventhal 2007-07-04 01:43:26 PDT
Comment on attachment 270841 [details] [diff] [review]
Patch rev. 1

Should nsHTMLComboboxAccessible::Shutdown() also manually shutdown the mComboboxTextFieldAccessible and mComboboxButtonAccessible  (same as you did for the list)?

+ mNextSibling = *aNextSibling;
I suggest moving both of those before the if, and changing it to:
SetNextSibling(accessible);
Comment 5 Mats Palmgren (:mats) 2007-07-04 10:35:49 PDT
Created attachment 270927 [details] [diff] [review]
Patch rev. 2

(In reply to comment #4)
> (From update of attachment 270841 [details] [diff] [review])
> Should nsHTMLComboboxAccessible::Shutdown() also manually shutdown the
> mComboboxTextFieldAccessible and mComboboxButtonAccessible

Sure.

> + mNextSibling = *aNextSibling;
> I suggest moving both of those before the if, and changing it to:
> SetNextSibling(accessible);

This makes me a bit nervous about it:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/base/nsAccessible.cpp&rev=1.165.2.15&root=/cvsroot&mark=370#368

DEAD_END_ACCESSIBLE == 0x1 and the combobox accessible code in general
doesn't look like it would behave well with mNextSibling == 0x1.
I'd prefer to leave the null-ptr logic in these methods as is for now -
I fear for regressions if we mess to much with it.
Comment 6 Daniel Veditz [:dveditz] 2007-07-06 16:15:34 PDT
Guessing at security impact. Would a user need to install something to be vulnerable to web content doing what this testcase does, or would simply turning on a screenreader or the like be sufficient?
Comment 7 Daniel Veditz [:dveditz] 2007-07-06 16:19:53 PDT
Comment on attachment 270927 [details] [diff] [review]
Patch rev. 2

approved for 1.8.1.5 and 1.8.0.13, a=dveditz
Comment 8 Mats Palmgren (:mats) 2007-07-08 14:57:43 PDT
(In reply to comment #6)
Daniel,  I think one can trick the AT into creating the right set of
a11y nodes from unprivileged script by moving around focus or selection
and then remove DOM nodes or restyle elements to cause the Shutdown()
to happen.  This would setup the necessary state for either accessing
a dead node (bug 387252) or to steal the children (this bug) through
a call to GetNextSibling().  So it comes down to if one can trick the AT
to do a GetNextSibling() on the right node from script.
It doesn't seem entirely impossible to me...  Aaron?

If my guess is right then simply turning on a screenreader or other AT
would be sufficient.
Comment 9 Mats Palmgren (:mats) 2007-07-08 16:23:24 PDT
MOZILLA_1_8_BRANCH:
mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp 	1.46.2.4 	mozilla/accessible/src/html/nsHTMLSelectAccessible.h 	1.28.2.2 

MOZILLA_1_8_0_BRANCH:
mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp 	1.46.2.2.2.2 	mozilla/accessible/src/html/nsHTMLSelectAccessible.h 	1.28.2.1.4.1 

(trunk not affected)

-> FIXED
Comment 10 Carsten Book [:Tomcat] 2007-08-23 04:45:06 PDT
Verified for 1.8.1.7 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070822 BonEcho/2.0.0.7pre ID:2007082203 with the testcase from comment #1 and verified for 1.8.0.13 using Thunderbird 1.5.0.13 RC with Thunderbrowse - no crash

Adding verified keywords
Comment 11 Daniel Veditz [:dveditz] 2008-02-08 14:23:28 PST
re-adding fixed1.8.1.5 to record when the actual fix happened, even though verification was later due to firedrills.

Note You need to log in before you can comment on or make changes to this bug.