Last Comment Bug 596232 - nsXULTemplateBuilder/nsXULTemplateQueryProcessorXML load new data sources at unsafe time
: nsXULTemplateBuilder/nsXULTemplateQueryProcessorXML load new data sources at ...
Status: RESOLVED FIXED
: [sg:critical?] for 1.9.2 and earlier
:
Product: Core
Classification: Components
Component: XUL
: unspecified
: x86 All
: -- normal (vote)
: ---
Assigned To: Neil Deakin
: xptoolkit.widgets
:
: 608687
:
  Show dependency treegraph
 
Reported: 2010-09-14 08:32 PDT by Olli Pettay [:smaug]
Modified: 2011-03-29 19:25 PDT (History)
8 users (show)
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  -
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
add checks (3.68 KB, patch)
2010-09-29 08:56 PDT, Neil Deakin
Olli.Pettay: review+
clegnitto: approval1.9.2.14+
Details | Diff | Splinter Review
trunk version (4.62 KB, patch)
2010-10-22 08:00 PDT, Neil Deakin
Olli.Pettay: review+
Details | Diff | Splinter Review
1.9.1 with extra include (4.18 KB, patch)
2011-01-21 08:50 PST, Neil Deakin
clegnitto: approval1.9.1.17+
Details | Diff | Splinter Review

Summon comment box

Description Olli Pettay [:smaug] 2010-09-14 08:32:07 PDT
nsEventDispatcher::Dispatch [content/events/src/nsEventDispatcher.cpp:515]
nsEventDispatcher::DispatchDOMEvent [content/events/src/nsEventDispatcher.cpp:691]
nsDOMEventTargetHelper::DispatchDOMEvent [content/events/src/nsDOMEventTargetHelper.cpp:230]
nsXMLHttpRequest::ChangeState [content/base/src/nsXMLHttpRequest.cpp:3000]
nsXMLHttpRequest::OpenRequest [content/base/src/nsXMLHttpRequest.cpp:1793]
nsXULTemplateQueryProcessorXML::GetDatasource [content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp:221]
nsXULTemplateBuilder::LoadDataSourceUrls [content/xul/templates/src/nsXULTemplateBuilder.cpp:1365]
nsXULTemplateBuilder::LoadDataSources [content/xul/templates/src/nsXULTemplateBuilder.cpp:1263]
nsXULTemplateBuilder::AttributeChanged [content/xul/templates/src/nsXULTemplateBuilder.cpp:1140]
nsXULContentBuilder::AttributeChanged [content/xul/templates/src/nsXULContentBuilder.cpp:1591]
nsNodeUtils::AttributeChanged [content/base/src/nsNodeUtils.cpp:127]
nsGenericElement::SetAttrAndNotify [content/base/src/nsGenericElement.cpp:4688]
nsGenericElement::SetAttr [content/base/src/nsGenericElement.cpp:4622]
nsGenericElement::SetAttribute [content/base/src/nsGenericElement.cpp:2400]
nsXULElement::SetAttribute [content/xul/content/src/nsXULElement.h:561]
nsIDOMElement_SetAttribute [dom_quickstubs.cpp:4918]

This bug is basically a clone of bug 553808.
Comment 1 Olli Pettay [:smaug] 2010-09-14 08:36:46 PDT
Not sure if web content can access this code on trunk, but on branches
it should be able to.

The following methods may have security problems
nsXULTemplateBuilder::AttributeChanged
nsXULTemplateBuilder::ContentRemoved
nsXULTemplateBuilder::NodeWillBeDestroyed
Comment 2 Olli Pettay [:smaug] 2010-09-20 13:53:50 PDT
Bug 553808 was assigned to you Neil. Might be safer to fix it here.

Sorry that I haven't fixed that bug, but the original tbox log hasif y long gone
and there were no new reports about it for awhile.

Anyway, Neil, you know this code a lot better than I do.
But just say if you don't have time. I could try to look at this too.
(But you'd have to review the patch carefully ;) )
Comment 3 Neil Deakin 2010-09-29 08:56:20 PDT
Created attachment 479407 [details] [review]
add checks
Comment 4 Daniel Veditz 2010-10-01 10:29:53 PDT
Comment on attachment 479407 [details] [review]
add checks

1.9.2.11 is past code-freeze, moving request to next release.
Comment 5 Daniel Veditz 2010-10-04 10:49:28 PDT
Comment on attachment 479407 [details] [review]
add checks

Approved for 1.9.2.12, a=dveditz for release-drivers
Comment 6 Damon Sicore (:damons) 2010-10-12 14:04:47 PDT
So, this just needs to be landed and tested/confirmed.
Comment 8 Damon Sicore (:damons) 2010-10-19 13:56:02 PDT
Can we close this?
Comment 9 Daniel Veditz 2010-10-19 13:58:37 PDT
Do we want to fix this problem on trunk as well? It may no longer be a security worry, but presumably we--or an addon--could stumble across this as a stability problem.

Does this patch work for trunk? Do we need a different patch for 1.9.1?

Marking status1.9.2 as .12-fixed per comment 7
Comment 10 Neil Deakin 2010-10-22 08:00:27 PDT
Created attachment 485290 [details] [review]
trunk version
Comment 12 Al Billings [:abillings] 2010-11-16 18:14:40 PST
Olli, is there anything for QA to do to verify this fix on 1.9.2?
Comment 13 Olli Pettay [:smaug] 2010-11-17 03:49:46 PST
You could ensure that loading test_tmpl_errors.xul in a debug build doesn't 
cause any assertions.
Comment 14 Daniel Veditz 2010-12-01 16:20:50 PST
trunk has a regression 608687 which got attached to the clone bug 553808 rather than here.
Comment 15 Daniel Veditz 2010-12-02 10:44:11 PST
Comment on attachment 479407 [details] [review]
add checks

>+      nsCOMPtr<nsIDocument> doc = mRoot ? mRoot->GetDocument() : nsnull;

Patch applies fine to 1.9.1 branch but fails to compile. The above results in

../../../../dist/include/xpcom/nsCOMPtr.h: In constructor ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsIDocument]’:
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:554: error: invalid use of undefined type ‘struct nsIDocument’
../../../../dist/include/content/nsNodeInfoManager.h:50: error: forward declaration of ‘struct nsIDocument’
../../../../dist/include/xpcom/nsCOMPtr.h:555: error: invalid static_cast from type ‘nsIDocument*’ to type ‘nsISupports*’
../../../../dist/include/xpcom/nsCOMPtr.h: In destructor ‘nsCOMPtr<T>::~nsCOMPtr() [with T = nsIDocument]’:
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:508: error: invalid static_cast from type ‘nsIDocument*’ to type ‘nsISupports*’
../../../../dist/include/xpcom/nsCOMPtr.h:510: error: invalid use of undefined type ‘struct nsIDocument’
../../../../dist/include/content/nsNodeInfoManager.h:50: error: forward declaration of ‘struct nsIDocument’
../../../../dist/include/xpcom/nsCOMPtr.h: In copy constructor ‘nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr<T>&) [with T = nsIDocument]’:
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:545: error: invalid use of undefined type ‘struct nsIDocument’
../../../../dist/include/content/nsNodeInfoManager.h:50: error: forward declaration of ‘struct nsIDocument’
../../../../dist/include/xpcom/nsCOMPtr.h:546: error: invalid static_cast from type ‘nsIDocument* const’ to type ‘nsISupports*’
../../../../dist/include/xpcom/nsCOMPtr.h: In member function ‘void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = nsIDocument]’:
../../../../dist/include/xpcom/nsCOMPtr.h:556:   instantiated from ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsIDocument]’
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:520: error: no matching function for call to ‘do_QueryInterface(nsIDocument*&)’
../../../../dist/include/xpcom/nsCOMPtr.h:303: note: candidates are: nsQueryInterface do_QueryInterface(nsISupports*)
../../../../dist/include/xpcom/nsCOMPtr.h:310: note:                 nsQueryInterfaceWithError do_QueryInterface(nsISupports*, nsresult*)
make[7]: *** [nsXULTemplateQueryProcessorStorage.o] Error 1
Comment 16 Daniel Veditz 2010-12-03 10:59:45 PST
Gavin backed this out from the 1.9.2 branch due to regression bug 608687
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d647705a2cf1 (relbranch)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8a23f17697a7
Comment 17 Christian Legnitto [:LegNeato] 2011-01-18 12:47:01 PST
Comment on attachment 479407 [details] [review]
add checks

a=LegNeato for 1.9.2.14
Comment 18 Christian Legnitto [:LegNeato] 2011-01-18 12:48:03 PST
We'll also need the fix for 1.9.1 as well...
Comment 19 Neil Deakin 2011-01-21 08:50:06 PST
Created attachment 505815 [details] [review]
1.9.1 with extra include
Comment 20 Christian Legnitto [:LegNeato] 2011-01-21 10:31:01 PST
Comment on attachment 505815 [details] [review]
1.9.1 with extra include

Approved for 1.9.1.17. Please land this as early as possible today so it makes the next release
Comment 22 Daniel Veditz 2011-01-21 11:55:49 PST
from Jan 18:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bc5bc0290118

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