Closed Bug 1364399 Opened 8 years ago Closed 7 years ago

###!!! ASSERTION: mOpQueue modified during tree op execution.: 'mFlushState == eNotFlushing', file /home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp, line 824

Categories

(Core :: DOM: HTML Parser, defect, P2)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: allstars.chh, Assigned: hsivonen)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(3 files, 4 obsolete files)

test_bug518104.html
1.09 KB, text/html
Details
Follow-up patch for later landing: test cases and clarifying comment
4.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Drop stream parser later, clean up how DidBuildModel is called, v4
10.47 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
Attached file test_bug518104.html —
See the attachment for the modifed dom/base/test/test_bug518104.html ./mach mochitest dom/base/test/test_bug518104.html The test passes, however it triggers assertion in the end. GECKO(26632) | [Child 26725] ###!!! ASSERTION: mOpQueue modified during tree op execution.: 'mFlushState == eNotFlushing', file /home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp, line 824 GECKO(26632) | #01: nsHtml5TreeOpExecutor::MoveOpsFrom(nsTArray<nsHtml5TreeOperation>&) (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp:824 (discriminator 1)) GECKO(26632) | #02: nsHtml5TreeBuilder::Flush(bool) (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeBuilderCppSupplement.h:1070) GECKO(26632) | #03: nsHtml5Parser::ParseUntilBlocked() (/home/allstars/src/gecko-dev/parser/html/nsHtml5Parser.cpp:656) GECKO(26632) | #04: nsHtml5Parser::Parse(nsAString const&, void*, nsACString const&, bool, nsDTDMode) (/home/allstars/src/gecko-dev/parser/html/nsHtml5Parser.cpp:274) GECKO(26632) | #05: nsHTMLDocument::Close(mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/dom/html/nsHTMLDocument.cpp:1824) GECKO(26632) | #06: mozilla::dom::HTMLDocumentBinding::close(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dom/bindings/HTMLDocumentBinding.cpp:611) GECKO(26632) | #07: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (/home/allstars/src/gecko-dev/dom/bindings/BindingUtils.cpp:2954) GECKO(26632) | #08: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/allstars/src/gecko-dev/js/src/jscntxtinlines.h:293) GECKO(26632) | #09: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:470) GECKO(26632) | #10: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:516) GECKO(26632) | #11: js::CallFromStack(JSContext*, JS::CallArgs const&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:522) GECKO(26632) | #12: Interpret(JSContext*, js::RunState&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:3025) GECKO(26632) | #13: js::RunScript(JSContext*, js::RunState&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:410) GECKO(26632) | #14: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:699) GECKO(26632) | #15: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:732) GECKO(26632) | #16: Evaluate(JSContext*, js::ScopeKind, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) (/home/allstars/src/gecko-dev/js/src/jsapi.cpp:4606 (discriminator 4)) GECKO(26632) | #17: Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) (/home/allstars/src/gecko-dev/js/src/jsapi.cpp:4630) GECKO(26632) | #18: JS::Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) (/home/allstars/src/gecko-dev/js/src/jsapi.cpp:4689) GECKO(26632) | #19: nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&) (/home/allstars/src/gecko-dev/dom/base/nsJSUtils.cpp:232) GECKO(26632) | #20: nsScriptLoader::EvaluateScript(nsScriptLoadRequest*) (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:2507) GECKO(26632) | #21: nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:2216) GECKO(26632) | #22: nsScriptLoader::ProcessPendingRequests() (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:2773) GECKO(26632) | #23: nsScriptLoader::ParsingComplete(bool) (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:3330) GECKO(26632) | #24: nsContentSink::DidBuildModelImpl(bool) (/home/allstars/src/gecko-dev/dom/base/nsContentSink.cpp:1508) GECKO(26632) | #25: nsHtml5TreeOpExecutor::DidBuildModel(bool) (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp:148 (discriminator 6)) GECKO(26632) | #26: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*) (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOperation.cpp:831) GECKO(26632) | #27: nsHtml5TreeOpExecutor::RunFlushLoop() (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp:458) GECKO(26632) | #28: nsHtml5ExecutorFlusher::Run() (/home/allstars/src/gecko-dev/parser/html/nsHtml5StreamParser.cpp:130) GECKO(26632) | #29: mozilla::SchedulerGroup::Runnable::Run() (/home/allstars/src/gecko-dev/xpcom/threads/SchedulerGroup.cpp:370) GECKO(26632) | #30: nsThread::ProcessNextEvent(bool, bool*) (/home/allstars/src/gecko-dev/xpcom/threads/nsThread.cpp:1251 (discriminator 2)) GECKO(26632) | #31: NS_ProcessNextEvent(nsIThread*, bool) (/home/allstars/src/gecko-dev/xpcom/threads/nsThreadUtils.cpp:393) GECKO(26632) | #32: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:96) GECKO(26632) | #33: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:302) GECKO(26632) | #34: MessageLoop::RunInternal() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:239) GECKO(26632) | #35: MessageLoop::RunHandler() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:232) GECKO(26632) | #36: MessageLoop::Run() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:210) GECKO(26632) | #37: nsBaseAppShell::Run() (/home/allstars/src/gecko-dev/widget/nsBaseAppShell.cpp:158) GECKO(26632) | #38: XRE_RunAppShell() (/home/allstars/src/gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:893) GECKO(26632) | #39: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:269) GECKO(26632) | #40: MessageLoop::RunInternal() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:239) GECKO(26632) | #41: MessageLoop::RunHandler() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:232) GECKO(26632) | #42: MessageLoop::Run() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:210) GECKO(26632) | #43: XRE_InitChildProcess(int, char**, XREChildData const*) (/home/allstars/src/gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:713) GECKO(26632) | #44: mozilla::BootstrapImpl::XRE_InitChildProcess(int, char**, XREChildData const*) (/home/allstars/src/gecko-dev/toolkit/xre/Bootstrap.cpp:66) GECKO(26632) | #45: content_process_main(mozilla::Bootstrap*, int, char**) (/home/allstars/src/gecko-dev/browser/app/../../ipc/contentproc/plugin-container.cpp:64) GECKO(26632) | #46: main (/home/allstars/src/gecko-dev/browser/app/nsBrowserApp.cpp:286) GECKO(26632) | #47: __libc_start_main (/build/glibc-cxyGtm/glibc-2.24/csu/../csu/libc-start.c:325) GECKO(26632) | #48: _start (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/firefox) GECKO(26632) | #49: ??? (???:???) GECKO(26632) | [Child 26725] ###!!! ASSERTION: nsHTMLDocument::Close(): Trying to remove nonexistent wyciwyg channel!: 'mWyciwygChannel', file /home/allstars/src/gecko-dev/dom/html/nsHTMLDocument.cpp, line 1861 GECKO(26632) | #01: nsHTMLDocument::Close(mozilla::ErrorResult&) (/home/allstars/src/gecko-dev/dom/html/nsHTMLDocument.cpp:1860 (discriminator 1)) GECKO(26632) | #02: mozilla::dom::HTMLDocumentBinding::close(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dom/bindings/HTMLDocumentBinding.cpp:611) GECKO(26632) | #03: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (/home/allstars/src/gecko-dev/dom/bindings/BindingUtils.cpp:2954) GECKO(26632) | #04: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/allstars/src/gecko-dev/js/src/jscntxtinlines.h:293) GECKO(26632) | #05: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:470) GECKO(26632) | #06: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:516) GECKO(26632) | #07: js::CallFromStack(JSContext*, JS::CallArgs const&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:522) GECKO(26632) | #08: Interpret(JSContext*, js::RunState&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:3025) GECKO(26632) | #09: js::RunScript(JSContext*, js::RunState&) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:410) GECKO(26632) | #10: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:699) GECKO(26632) | #11: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (/home/allstars/src/gecko-dev/js/src/vm/Interpreter.cpp:732) GECKO(26632) | #12: Evaluate(JSContext*, js::ScopeKind, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) (/home/allstars/src/gecko-dev/js/src/jsapi.cpp:4606 (discriminator 4)) GECKO(26632) | #13: Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) (/home/allstars/src/gecko-dev/js/src/jsapi.cpp:4630) GECKO(26632) | #14: JS::Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) (/home/allstars/src/gecko-dev/js/src/jsapi.cpp:4689) GECKO(26632) | #15: nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&) (/home/allstars/src/gecko-dev/dom/base/nsJSUtils.cpp:232) GECKO(26632) | #16: nsScriptLoader::EvaluateScript(nsScriptLoadRequest*) (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:2507) GECKO(26632) | #17: nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:2216) GECKO(26632) | #18: nsScriptLoader::ProcessPendingRequests() (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:2773) GECKO(26632) | #19: nsScriptLoader::ParsingComplete(bool) (/home/allstars/src/gecko-dev/dom/base/nsScriptLoader.cpp:3330) GECKO(26632) | #20: nsContentSink::DidBuildModelImpl(bool) (/home/allstars/src/gecko-dev/dom/base/nsContentSink.cpp:1508) GECKO(26632) | #21: nsHtml5TreeOpExecutor::DidBuildModel(bool) (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp:148 (discriminator 6)) GECKO(26632) | #22: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*) (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOperation.cpp:831) GECKO(26632) | #23: nsHtml5TreeOpExecutor::RunFlushLoop() (/home/allstars/src/gecko-dev/parser/html/nsHtml5TreeOpExecutor.cpp:458) GECKO(26632) | #24: nsHtml5ExecutorFlusher::Run() (/home/allstars/src/gecko-dev/parser/html/nsHtml5StreamParser.cpp:130) GECKO(26632) | #25: mozilla::SchedulerGroup::Runnable::Run() (/home/allstars/src/gecko-dev/xpcom/threads/SchedulerGroup.cpp:370) GECKO(26632) | #26: nsThread::ProcessNextEvent(bool, bool*) (/home/allstars/src/gecko-dev/xpcom/threads/nsThread.cpp:1251 (discriminator 2)) GECKO(26632) | #27: NS_ProcessNextEvent(nsIThread*, bool) (/home/allstars/src/gecko-dev/xpcom/threads/nsThreadUtils.cpp:393) GECKO(26632) | #28: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:96) GECKO(26632) | #29: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:302) GECKO(26632) | #30: MessageLoop::RunInternal() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:239) GECKO(26632) | #31: MessageLoop::RunHandler() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:232) GECKO(26632) | #32: MessageLoop::Run() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:210) GECKO(26632) | #33: nsBaseAppShell::Run() (/home/allstars/src/gecko-dev/widget/nsBaseAppShell.cpp:158) GECKO(26632) | #34: XRE_RunAppShell() (/home/allstars/src/gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:893) GECKO(26632) | #35: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:269) GECKO(26632) | #36: MessageLoop::RunInternal() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:239) GECKO(26632) | #37: MessageLoop::RunHandler() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:232) GECKO(26632) | #38: MessageLoop::Run() (/home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:210) GECKO(26632) | #39: XRE_InitChildProcess(int, char**, XREChildData const*) (/home/allstars/src/gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:713) GECKO(26632) | #40: mozilla::BootstrapImpl::XRE_InitChildProcess(int, char**, XREChildData const*) (/home/allstars/src/gecko-dev/toolkit/xre/Bootstrap.cpp:66) GECKO(26632) | #41: content_process_main(mozilla::Bootstrap*, int, char**) (/home/allstars/src/gecko-dev/browser/app/../../ipc/contentproc/plugin-container.cpp:64) GECKO(26632) | #42: main (/home/allstars/src/gecko-dev/browser/app/nsBrowserApp.cpp:286) GECKO(26632) | #43: __libc_start_main (/build/glibc-cxyGtm/glibc-2.24/csu/../csu/libc-start.c:325) GECKO(26632) | #44: _start (/home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/firefox) GECKO(26632) | #45: ??? (???:???)
Hi Henri Can you check the assertion failure and give me some suggestions? I've pinged smaug and he said I should ask you for more information. Thanks
Flags: needinfo?(hsivonen)
There's a bug in how eTreeOpStreamEnded is processed in nsHtml5TreeOperation.cpp. Instead of calling aBuilder->DidBuildModel(false); directly, it should set a flag on aBuilder and then nsHtml5TreeOpExecutor::RunFlushLoop() should check for the flag and call DidBuildModel(false); outside the loop at the end of the method if the flag is set.
Flags: needinfo?(hsivonen)
Comment on attachment 8868457 [details] [diff] [review] WIP patch Review of attachment 8868457 [details] [diff] [review]: ----------------------------------------------------------------- Hi Henri I did as your comment 2 said, but it isn't working, Can you check did I miss anything? Thanks
Attachment #8868457 - Flags: feedback?(hsivonen)
Comment on attachment 8868457 [details] [diff] [review] WIP patch This is exactly what I meant. I'm surprised that it doesn't work. Do you mean the symptoms are the same as before or are there now different adverse symptoms?
Attachment #8868457 - Flags: feedback?(hsivonen) → feedback+
(In reply to Henri Sivonen (:hsivonen) from comment #5) > Comment on attachment 8868457 [details] [diff] [review] > WIP patch > > This is exactly what I meant. I'm surprised that it doesn't work. > > Do you mean the symptoms are the same as before or are there now different > adverse symptoms? New differnt problem, now even the test doesn't run, and it seems to be an idle state. Perhaps I need more time to find out the problem, if you have some suggestions feel free to let me know :)
Priority: -- → P2
Hi Yoshi, What's the current status of this bug? Do we still need to fix it?
Flags: needinfo?(allstars.chh)
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(allstars.chh)
Resolution: --- → WONTFIX
Why WONTFIX? This assertion firing is quite concerning and, I think, worth investigating at least on *some* schedule.
Flags: needinfo?(allstars.chh)
Sorry,I don't know the status, my original plan was to fix bug 1363629, but now since ckerschb and smaug took over that bug and that bug is landed, so I guess somehow this bug is fixed already. Feel free to reopen it if you think it still exists.
Flags: needinfo?(allstars.chh)
Reopening, since it appears the test case was changed instead of fixing the parser.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Group: core-security
Assignee: nobody → hsivonen
Status: REOPENED → ASSIGNED
Root cause: document.close() checks for script-createdness of the parser. A parser is not script-created iff it has a stream parser. However, upon EOF, nsHtml5Parser drops its nsHtml5StreamParser before the nsHtml5Parser becomes unreachable from nsHTMLDocument, so a parser that should count as not script-created looks like a script-created parser for a moment. During that moment, async scripts can run if they have been fetched by the time the EOF of the HTML document is seen. Solution: We must unlink nsHtml5StreamParser from nsHtml5Parser *after* we've unlinked nsHtml5Parser from nsHTMLDocument.
Pushed to try with clean-up diversion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c27a58a10b6a2eb87f935d07af091db51f701b47 (Not really feasible to land a patch like this without a try run. The actual fix is the GetParser()->DropStreamParser(); line moving down.)
The real fix is dropping the stream parser after nsContentSink::DidBuildModel. See comment 11. The way nsHtml5TreeOpExecutor::DidBuildModel is called turned out to be just clean-up, but I left it in the patch as a diversion for people who watch our security landings. I'll write a follow-up patch with comments and the test case.
Attachment #8868457 - Attachment is obsolete: true
Attachment #8923700 - Flags: review?(bugs)
This time without trailing whitespace.
Attachment #8923700 - Attachment is obsolete: true
Attachment #8923700 - Flags: review?(bugs)
Attachment #8923702 - Flags: review?(bugs)
Filed bug 1413101 as a follow-up.
Note for the purpose of giving this a security rating: The assertion that's triggered here normally indicates that the code is about to potentially do the "free" part of use-after-free. In this case, however, it *seems* to me that this happens when the last item of the array potentially about to be freed has already been read, so it *seems* like the "use" part of UAF is averted in this case. However, I'm not 100% sure that there doesn't exist a way to have an actual UAF here under more complex circumstances than I can think about. AFAICT, this bug does potentially involve what's technically UB (comparing a pointer that no longer points to a valid object to exit the loop that iterated over the array that maybe got freed after its last item was read but before the loop terminated), but in this case the compiler shouldn't be able to know that and do any UB-based transformations.
Attachment #8923706 - Flags: review?(bugs) → review+
in general, it would be easier to read patches if they were created with -U 8.
(In reply to Henri Sivonen (:hsivonen) from comment #15) > Created attachment 8923700 [details] [diff] [review] > Drop stream parser later, clean up how DidBuildModel is called > > The real fix is dropping the stream parser after > nsContentSink::DidBuildModel. See comment 11. I don't understand this. drop stream parser after what?
Comment on attachment 8923702 [details] [diff] [review] Drop stream parser later, clean up how DidBuildModel is called, v2 > NS_IMETHODIMP > nsHtml5TreeOpExecutor::DidBuildModel(bool aTerminated) > { >- if (!aTerminated) { >- // This is needed to avoid unblocking loads too many times on one hand >- // and on the other hand to avoid destroying the frame constructor from >- // within an update batch. See bug 537683. >- EndDocUpdate(); Why we can drop this call now?
Could you explain the patch a bit.
Flags: needinfo?(hsivonen)
Comment on attachment 8923702 [details] [diff] [review] Drop stream parser later, clean up how DidBuildModel is called, v2 > nsHtml5TreeOpExecutor::DidBuildModel(bool aTerminated) > { ok, based on IRC discussion, please MOZ_ASSERT here that we are actually in an update.
Attachment #8923702 - Flags: review?(bugs) → review+
Thank you. Added an assertion. Carrying forward r+.
Attachment #8923702 - Attachment is obsolete: true
Flags: needinfo?(hsivonen)
Attachment #8923857 - Flags: review+
Comment on attachment 8923857 [details] [diff] [review] Drop stream parser later, clean up how DidBuildModel is called, v3 [Security approval request comment] > How easily could an exploit be constructed based on the patch? Most of the patch distracts from the real fix, so spotting the real fix is not obvious. It's not clear that there even is an exploit to be constructed. (See comment 19.) > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. (They are in the follow-up patch.) > Which older supported branches are affected by this flaw? All. > If not all supported branches, which bug introduced the flaw? Probably goes back to Firefox 4. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I have not created backports, but creating a backport for ESR 52 shouldn't be hard. If this patch survives on Nightly for a few days, a backport should not be too risky. > How likely is this patch to cause regressions; how much testing does it need? It could have subtle problems but it's not likely and if there are problems, they are likely to be in edge cases that wouldn't affect most sites. The fix passes our test suite. The idea is to land this patch together with the patch for bug 1410848.
Attachment #8923857 - Flags: sec-approval?
Talking to Dan, we think this is a sec-moderate issue. As such, it doesn't need sec-approval to go into mozilla-central, so I'm going to clear the request. Just check it into trunk.
Keywords: sec-moderate
Attachment #8923857 - Flags: sec-approval?
Group: core-security → dom-core-security
Keywords: csectype-uaf
Fixing the condition of the assertion added after review in response to review comment. (Unlike I claimed on IRC yesterday, the method can be called from within a flush from an update batch pause (i.e. not from within an update batch) when the parser is stopped by JS before the parse is complete, i.e. when aTerminated is true. However, the old removed bit of code was conditional on aTerminated being false, so its removal doesn't fail to account for any situation that wasn't accounted for previously.)
Attachment #8923857 - Attachment is obsolete: true
Attachment #8924116 - Flags: review+
> wasn't accounted s/wasn't/was/
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/388ffddeb4623b932ed67554f2ddc434b1fed8fb(In reply to Al Billings [:abillings] from comment #27) > Talking to Dan, we think this is a sec-moderate issue. As such, it doesn't > need sec-approval to go into mozilla-central, so I'm going to clear the > request. Just check it into trunk. Thanks. Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/388ffddeb4623b932ed67554f2ddc434b1fed8fb Any advice on when to land the follow-up patch that adds the test case and a comment that explains the problem?
Flags: needinfo?(abillings)
Adding the in-testsuite? flag to remind us to check in the second part with tests after we ship the fix.
Flags: in-testsuite?
Henri, we should hold off on checking in tests for a security bug until the fix has been shipped to the public.
Flags: needinfo?(abillings)
https://hg.mozilla.org/mozilla-central/rev/388ffddeb462 Not clear to me from comment 26 if this is something we want to backport to affected branches or just let ride the trains. Please request approval or set flags to wontfix accordingly :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → fixed
status-firefox-esr52: --- → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: dom-core-security → core-security-release
Depends on: 1414490
This needs a backout (and re-fixing differently) for causing bug 1414490.
(In reply to Henri Sivonen (:hsivonen) from comment #37) > This needs a backout (and re-fixing differently) for causing bug 1414490. Nevermind. I'll fix bug 1414490 without backing this out first.
Depends on: 1418000
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
The test case could land now.
Flags: needinfo?(hsivonen)
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9623f9b14d93 follow-up - Test cases and clarifying comment. r=smaug.
Flags: needinfo?(hsivonen)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: