Bugzilla@Mozilla – Bug 596232
nsXULTemplateBuilder/nsXULTemplateQueryProcessorXML load new data sources at unsafe time
Last modified: 2011-03-29 19:25:15 PDT
Summon comment box
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.
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
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 ;) )
Created attachment 479407 [details] [review] add checks
Comment on attachment 479407 [details] [review] add checks 1.9.2.11 is past code-freeze, moving request to next release.
Comment on attachment 479407 [details] [review] add checks Approved for 1.9.2.12, a=dveditz for release-drivers
So, this just needs to be landed and tested/confirmed.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c7a429d62c81
Can we close this?
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
Created attachment 485290 [details] [review] trunk version
http://hg.mozilla.org/mozilla-central/rev/3e08f8844f87
Olli, is there anything for QA to do to verify this fix on 1.9.2?
You could ensure that loading test_tmpl_errors.xul in a debug build doesn't cause any assertions.
trunk has a regression 608687 which got attached to the clone bug 553808 rather than here.
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
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 on attachment 479407 [details] [review] add checks a=LegNeato for 1.9.2.14
We'll also need the fix for 1.9.1 as well...
Created attachment 505815 [details] [review] 1.9.1 with extra include
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
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6d4894752304
from Jan 18: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bc5bc0290118