The attached testcase causes a use-after-free crash in m-c rev 44121dbcac6a with stylo enabled by pref. Either a fuzzing build [1] with fuzzing.enabled=true, or the domFuzzLite extension [2] are required to trigger cycle-collection. 1. https://tools.taskcluster.net/index/artifacts/gecko.v2.mozilla-central.latest.firefox/linux64-fuzzing-asan-opt 2. https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension ==32685==ERROR: AddressSanitizer: heap-use-after-free on address 0x610000053a90 at pc 0x7f52927c2bd2 bp 0x7f523c4931d0 sp 0x7f523c4931c8 READ of size 8 at 0x610000053a90 thread T29 (StyleThread#1) #0 0x7f52927c2bd1 in GetExistingSlots /home/worker/workspace/build/src/dom/base/nsINode.h:1945:12 #1 0x7f52927c2bd1 in GetExistingDOMSlots /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FragmentOrElement.h:390 #2 0x7f52927c2bd1 in GetExistingExtendedDOMSlots /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FragmentOrElement.h:405 #3 0x7f52927c2bd1 in mozilla::dom::Element::GetSMILOverrideStyleDeclaration() /home/worker/workspace/build/src/dom/base/Element.cpp:2027 #4 0x7f529779fc48 in Gecko_GetSMILOverrideDeclarationBlock /home/worker/workspace/build/src/layout/style/ServoBindings.cpp:496:42 #5 0x7f529d9b31c1 in style::gecko::wrapper::{{impl}}::get_smil_override /home/worker/workspace/build/src/servo/components/style/gecko/wrapper.rs:883 #6 0x7f529d9b31c1 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::match_primary::hf9c108ef69a03273 /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:340 #7 0x7f529d9aadf7 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_primary_style::h865e57febbb64917 /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:96 #8 0x7f529d9a7617 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_style::hf238718fcb092dbe /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:144 #9 0x7f529d9a1262 in style::style_resolver::{{impl}}::resolve_style_with_default_parents::{{closure}}<style::gecko::wrapper::GeckoElement> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:178 #10 0x7f529d9a1262 in style::style_resolver::with_default_parent_styles<style::gecko::wrapper::GeckoElement,closure,style::data::ElementStyles> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:66 #11 0x7f529d9a1262 in style::style_resolver::{{impl}}::resolve_style_with_default_parents<style::gecko::wrapper::GeckoElement> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:177 #12 0x7f529d9a1262 in style::traversal::compute_style::h00a4eb976c467bf8 /home/worker/workspace/build/src/servo/components/style/traversal.rs:684 #13 0x7f529d9bd040 in style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure> /home/worker/workspace/build/src/servo/components/style/traversal.rs:501 #14 0x7f529d9bd040 in style::gecko::traversal::{{impl}}::process_preorder<closure> /home/worker/workspace/build/src/servo/components/style/gecko/traversal.rs:39 #15 0x7f529d9bd040 in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /home/worker/workspace/build/src/servo/components/style/parallel.rs:205 #16 0x7f529d9bd040 in style::parallel::traverse_nodes::h43680bf12ea94af1 /home/worker/workspace/build/src/servo/components/style/parallel.rs:283 #17 0x7f529d9bd570 in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /home/worker/workspace/build/src/servo/components/style/parallel.rs:221 #18 0x7f529d9bd570 in style::parallel::traverse_nodes::h43680bf12ea94af1 /home/worker/workspace/build/src/servo/components/style/parallel.rs:283 #19 0x7f529d9bc713 in style::parallel::traverse_dom::{{closure}}::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /home/worker/workspace/build/src/servo/components/style/parallel.rs:92 #20 0x7f529d9bc713 in rayon_core::scope::{{impl}}::execute_job_closure::{{closure}}<closure,()> /home/worker/workspace/build/src/third_party/rust/rayon-core/src/scope/mod.rs:354 #21 0x7f529d9bc713 in std::panic::{{impl}}::call_once<(),closure> /checkout/src/libstd/panic.rs:296 #22 0x7f529d9bc713 in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> /checkout/src/libstd/panicking.rs:454 #23 0x7f529d9bc713 in panic_abort::__rust_maybe_catch_panic /checkout/src/libpanic_abort/lib.rs:40

It appears that the <a> element (generated by turning on designMode on a table) has been freed.

(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #2) > It appears that the <a> element (generated by turning on designMode on a > table) has been freed. Is the <a> element some sort of NAC? If so, then perhaps the issue is that we're finding that element via the frame tree (via the StyleChildrenIterator), but the frame has a stale reference to the dead element?

Yeah, it seems to be created when HTMLEditor::ShowInlineTableEditingUI calls HTMLEditor::CreateAnonymousElement[1] [1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/editor/libeditor/HTMLInlineTableEditor.cpp#64

Hm, so it looks like the StyleChildrenIterator finds this NAC via the manualNACProperty machinery. So this is the machinery that got added in bug 1374999. Assuming that's correct, then this is orthogonal to the frame tree, and just a question of how we manage to drop the ref to the NAC without also de-registering it in DeleteRefToAnonymousNode. Maybe this is some edge-case where the PresShell is null? DeleteRefToAnonymousNode should spit out a warning in that case. As belt-and-suspenders, we should probably make ManualNAC hold strong references to the elements. Brian, are you going to continue investigating this?

No, sorry I have been at standards meetings all week and am away from now until mid-next week.

Ok, I'll take it then.

OK, so the existing code is only correct if HTMLEditor::HideInlineTableEditingUI is called before the destructor runs, but it isn't. This is a regression from bug 1374999.

(This probably affects non-stylo as well)

This is belt-and-suspenders. It transforms the current UAF into a leak, which is strictly preferable. MozReview-Commit-ID: H1W7RZeGQYy

MozReview-Commit-ID: BxbmWjovaHm

MozReview-Commit-ID: DGFMbL3Djrq

Comment on attachment 8893112 [details] [diff] [review] Part 1 - Use a strong reference for ManualNAC. v1 I'm not a good person to review nsContentUtils. Please look for better reviewer.

Comment on attachment 8893112 [details] [diff] [review] Part 1 - Use a strong reference for ManualNAC. v1 Ah, but it'll be moved to editor. Okay, I'll review this.

(You can also ask heycam, who added the original ManualNAC machinery - I just figured that you might want to review the new setup for editor).

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=d6f9510a73a14aa0905e7dece30657318d6b13cc Isn't it a bad practice to push patch of sec-critical bug to try server before getting sec-approval? https://wiki.mozilla.org/Security/Bug_Approval_Process#Pushing_to_Try

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=d6f9510a73a14aa0905e7dece30657318d6b13cc > > Isn't it a bad practice to push patch of sec-critical bug to try server > before getting sec-approval? > > https://wiki.mozilla.org/Security/Bug_Approval_Process#Pushing_to_Try I don't think it's realistic to develop complex patches against mozilla-central without pushing to try. I certainly could have obfuscated it more, and not pushed the crashtest. But since it doesn't affect release and I expect to have the patch landed within a day or two, I'm not particularly worried about it.

Comment on attachment 8893114 [details] [diff] [review] Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v1 ># HG changeset patch ># User Bobby Holley <bobbyholley@gmail.com> > >Bug 1386110 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v1 > >MozReview-Commit-ID: BxbmWjovaHm > >diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp >index 8173c1d6..f8dfabf 100644 >--- a/dom/base/nsContentUtils.cpp >+++ b/dom/base/nsContentUtils.cpp >@@ -33,16 +33,17 @@ > #include "mozilla/ArrayUtils.h" > #include "mozilla/Attributes.h" > #include "mozilla/AutoRestore.h" > #include "mozilla/AutoTimelineMarker.h" > #include "mozilla/Base64.h" > #include "mozilla/CheckedInt.h" > #include "mozilla/DebugOnly.h" > #include "mozilla/LoadInfo.h" >+#include "mozilla/ManualNAC.h" > #include "mozilla/dom/ContentChild.h" nit: This file includes header files a-z order of the *path*. So, "mozilla/Ma" should be inserted to below |#include "mozilla/Likely.h"|. >diff --git a/editor/libeditor/ManualNAC.h b/editor/libeditor/ManualNAC.h >+#ifndef mozilla_ManualNAC_h >+#define mozilla_ManualNAC_h >+ >+#include "mozilla/dom/Element.h" I think that you need to include RefPtr.h too for avoiding include hell. >+ >+namespace mozilla { >+ >+// 16 seems to be the maximum number of manual NAC nodes that editor >+// creates for a given element. I don't know what "NAC" means. If you know, could you add the original words with parentheses? (I wonder, "NAC" is "Native Anonymous Content"?) >+// >+// These need to be manually removed by the machinery that sets the NAC, >+// otherwise we'll leak. >+typedef AutoTArray<RefPtr<mozilla::dom::Element>,16> ManualNACArray; nit: please insert a whitespace before "16". And perhaps, "mozilla::" isn't necessary here. >+ >+class ManualNACPtr final? And please add explanation of the purpose of this class before this line. >+{ >+public: >+ ManualNACPtr() {} >+ ManualNACPtr(decltype(nullptr)) {} >+ ManualNACPtr(already_AddRefed<Element> aNewNAC) Shouldn't be a reference? >+ // We use move semantics, and delete the copy-constructor and operator=. >+ ManualNACPtr(ManualNACPtr&& aOther) : mPtr(aOther.mPtr.forget()) {} >+ ManualNACPtr(ManualNACPtr& aOther) = delete; >+ ManualNACPtr& operator=(ManualNACPtr&& aOther) { mPtr = aOther.mPtr.forget(); return *this; } too long line. Plase wrap after |aOther)|, |{|, |forget();|, |*this;| and |}|. >+ Element* Get() const { return mPtr.get(); } nit: |get()| may be better than |Get()| for consistency with RefPtr and nsCOMPtr. >+ Element* operator->() const { return Get(); } >+ operator Element*() const & >+ { >+ return Get(); >+ } >+ >+private: >+ RefPtr<Element> mPtr; >+}; >+ >+} // namespace mozilla >+ >+inline void >+ImplCycleCollectionUnlink(mozilla::ManualNACPtr& field) >+{ >+ field.Reset(); Use 2 spaces for indent. >+} >+ >+inline void >+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& callback, >+ const mozilla::ManualNACPtr& field, >+ const char* name, >+ uint32_t flags = 0) >+{ >+ CycleCollectionNoteChild(callback, field.Get(), name, flags); Use 2 spaces for indent. >diff --git a/editor/libeditor/HTMLAbsPositionEditor.cpp b/editor/libeditor/HTMLAbsPositionEditor.cpp >@@ -284,20 +284,18 @@ HTMLEditor::HideGrabber() > NS_ENSURE_TRUE(mGrabber, NS_ERROR_NULL_POINTER); > > // get the presshell's document observer interface. > nsCOMPtr<nsIPresShell> ps = GetPresShell(); > // We allow the pres shell to be null; when it is, we presume there > // are no document observers to notify, but we still want to > // UnbindFromTree. > >- DeleteRefToAnonymousNode(mGrabber, ps); >- mGrabber = nullptr; >- DeleteRefToAnonymousNode(mPositioningShadow, ps); >- mPositioningShadow = nullptr; >+ DeleteRefToAnonymousNode(Move(mGrabber), ps); >+ DeleteRefToAnonymousNode(Move(mPositioningShadow), ps); If the caller forgets to use Move(), will it cause compile error due to no copy constructor? I worry about easy mistake of other developers including me. I'd like to check new patch, temporarily r-.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #19) > nit: This file includes header files a-z order of the *path*. So, > "mozilla/Ma" should be inserted to below |#include "mozilla/Likely.h"|. Fixed. > > >diff --git a/editor/libeditor/ManualNAC.h b/editor/libeditor/ManualNAC.h > >+#ifndef mozilla_ManualNAC_h > >+#define mozilla_ManualNAC_h > >+ > >+#include "mozilla/dom/Element.h" > > I think that you need to include RefPtr.h too for avoiding include hell. Fixed. > > >+ > >+namespace mozilla { > >+ > >+// 16 seems to be the maximum number of manual NAC nodes that editor > >+// creates for a given element. > > I don't know what "NAC" means. If you know, could you add the original words > with parentheses? Fixed. > > (I wonder, "NAC" is "Native Anonymous Content"?) Yes. > > >+// > >+// These need to be manually removed by the machinery that sets the NAC, > >+// otherwise we'll leak. > >+typedef AutoTArray<RefPtr<mozilla::dom::Element>,16> ManualNACArray; > > nit: please insert a whitespace before "16". And perhaps, "mozilla::" isn't > necessary here. Fixed. > > >+ > >+class ManualNACPtr > > final? Sure. > > And please add explanation of the purpose of this class before this line. Done. > > >+{ > >+public: > >+ ManualNACPtr() {} > >+ ManualNACPtr(decltype(nullptr)) {} > >+ ManualNACPtr(already_AddRefed<Element> aNewNAC) > > Shouldn't be a reference? Hm, is that preferable? Seems like that just allows aliasing the already_AddRefed, whereas passing by value requires the caller to use already_AddRefed's move-constructor, either by passing an anonymous result or by explicitly Move()-ing a value. > > >+ // We use move semantics, and delete the copy-constructor and operator=. > >+ ManualNACPtr(ManualNACPtr&& aOther) : mPtr(aOther.mPtr.forget()) {} > >+ ManualNACPtr(ManualNACPtr& aOther) = delete; > >+ ManualNACPtr& operator=(ManualNACPtr&& aOther) { mPtr = aOther.mPtr.forget(); return *this; } > > too long line. Plase wrap after |aOther)|, |{|, |forget();|, |*this;| and > |}|. Fixed. > > >+ Element* Get() const { return mPtr.get(); } > > nit: |get()| may be better than |Get()| for consistency with RefPtr and > nsCOMPtr. Ok. > > >+ Element* operator->() const { return Get(); } > >+ operator Element*() const & > >+ { > >+ return Get(); > >+ } > >+ > >+private: > >+ RefPtr<Element> mPtr; > >+}; > >+ > >+} // namespace mozilla > >+ > >+inline void > >+ImplCycleCollectionUnlink(mozilla::ManualNACPtr& field) > >+{ > >+ field.Reset(); > > Use 2 spaces for indent. Fixed (was cargo-culting from another file). > > >+} > >+ > >+inline void > >+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& callback, > >+ const mozilla::ManualNACPtr& field, > >+ const char* name, > >+ uint32_t flags = 0) > >+{ > >+ CycleCollectionNoteChild(callback, field.Get(), name, flags); > > Use 2 spaces for indent. Fixed. > > >diff --git a/editor/libeditor/HTMLAbsPositionEditor.cpp b/editor/libeditor/HTMLAbsPositionEditor.cpp > >@@ -284,20 +284,18 @@ HTMLEditor::HideGrabber() > > NS_ENSURE_TRUE(mGrabber, NS_ERROR_NULL_POINTER); > > > > // get the presshell's document observer interface. > > nsCOMPtr<nsIPresShell> ps = GetPresShell(); > > // We allow the pres shell to be null; when it is, we presume there > > // are no document observers to notify, but we still want to > > // UnbindFromTree. > > > >- DeleteRefToAnonymousNode(mGrabber, ps); > >- mGrabber = nullptr; > >- DeleteRefToAnonymousNode(mPositioningShadow, ps); > >- mPositioningShadow = nullptr; > >+ DeleteRefToAnonymousNode(Move(mGrabber), ps); > >+ DeleteRefToAnonymousNode(Move(mPositioningShadow), ps); > > If the caller forgets to use Move(), will it cause compile error due to no > copy constructor? That's right, since the copy-constructor is explicitly |delete|.

MozReview-Commit-ID: HTSu5BjxD8I

Comment on attachment 8893501 [details] [diff] [review] Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v2 Flagging sec approval for both patches in the bug, since I expect v2 to be r+ed. [Security approval request comment] How easily could an exploit be constructed based on the patch? Not super-easily, would need to figure out how to trigger the crash, and then exploit it. Crashtest does the first, but we can wait to push that (accidentally pushed it to try though). Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It's probably clear that we're dealing with UAF. Which older supported branches are affected by this flaw? 56 and 57. Not 55. If not all supported branches, which bug introduced the flaw? bug 1386110. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Low risk, same patch should apply. How likely is this patch to cause regressions; how much testing does it need? We should land it on Nightly, wait a day or two to make sure there's no major fallout, then land to beta.

Comment on attachment 8893501 [details] [diff] [review] Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v2 >> >+{ >> >+public: >> >+ ManualNACPtr() {} >> >+ ManualNACPtr(decltype(nullptr)) {} >> >+ ManualNACPtr(already_AddRefed<Element> aNewNAC) >> >> Shouldn't be a reference? > > Hm, is that preferable? Seems like that just allows aliasing the > already_AddRefed, whereas passing by value requires the caller to use > already_AddRefed's move-constructor, either by passing an anonymous result or > by explicitly Move()-ing a value. I thought that not using reference causes unnecessary |.take()| is called. If it's not matter or it's my misunderstanding, keep using this style. (Anyway, really a small issue.)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #23) > I thought that not using reference causes unnecessary |.take()| is called. In the sense that the Move constructor is invoked where it otherwise wouldn't be? That is true, but I'm pretty sure that modern compilers can optimize out the actual invocation of the Move constructor in cases like this (since it's just shuffling the pointer between temporaries, with no refcounting). Let me know if we have any evidence that it does add overhead. Thanks for the reviews!

Thank you for fixing this Bobby. BTW you probably need some explicit / MOZ_IMPLICT annotations on the ManualNACPtr constructors. (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22) > If not all supported branches, which bug introduced the flaw? > > bug 1386110. Probably you mean bug 1374999.

Comment on attachment 8893501 [details] [diff] [review] Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v2 sec-approval= dveditz

MozReview-Commit-ID: HTSu5BjxD8I

Comment on attachment 8893651 [details] [diff] [review] Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. r=masayuki Applies to Part 1 + Part 2. Approval Request Comment [Feature/Bug causing the regression]: bug 1374999 [User impact if declined]: UAF [Is this code covered by automated tests?]: Yes (crashtest in the bug) [Has the fix been verified in Nightly?]: Not yet (just pushed) [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: Part 1 + Part 2 together [Is the change risky?]: medium-low. [Why is the change risky/not risky?]: Reshuffles some editor internals, but the behavior should be the same. [String changes made/needed]: None

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #24) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #23) > > I thought that not using reference causes unnecessary |.take()| is called. > > In the sense that the Move constructor is invoked where it otherwise > wouldn't be? That is true, but I'm pretty sure that modern compilers can > optimize out the actual invocation of the Move constructor in cases like > this (since it's just shuffling the pointer between temporaries, with no > refcounting). Let me know if we have any evidence that it does add overhead. > > Thanks for the reviews! No problem. Thank you for your explanation. I still don't understand around Move of C++11 deeply.

https://hg.mozilla.org/mozilla-central/rev/72756e10985d https://hg.mozilla.org/mozilla-central/rev/e79cd7f08b4f

Comment on attachment 8893112 [details] [diff] [review] Part 1 - Use a strong reference for ManualNAC. v1 Crash fix, sec issue, let's uplift this for 56 beta 2.

https://hg.mozilla.org/releases/mozilla-beta/rev/6efea60c6783 https://hg.mozilla.org/releases/mozilla-beta/rev/60c17d37ab38

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #28) > [Is this code covered by automated tests?]: Yes (crashtest in the bug) > [Has the fix been verified in Nightly?]: Not yet (just pushed) > [Needs manual test from QE? If yes, steps to reproduce]: No. Setting qe-verify- based on Bobby's assessment on manual testing needs.