New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 697536 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

!ScriptForbiddenScope::isScriptForbidden() in V8PerIsolateData.cpp

Project Member Reported by ClusterFuzz, Mar 1 2017

Issue description

Components: Blink>JavaScript
Labels: Test-Predator-Wrong M-58
Cc: haraken@chromium.org jochen@chromium.org
Owner: dgozman@chromium.org
Status: Assigned (was: Untriaged)
The #17 blink::Document::updateStyleAndLayout() sets up a ScriptForbiddenScope (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?rcl=d8e518533a6a987997cdcf438ddc21573eb75de0&l=2191),
then the stack overflow happens and call a V8 to throw the exception (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp?rcl=122c4da76a70443fe965d1413741a79416a6e763&l=137)
and finally we hit this assert: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp?rcl=9b5ea61b97315d1d028f440d6dc7defed009cf66&l=48.

This is the call stack:

#0 0x0000045a3a2b base::debug::StackTrace::StackTrace()
#1 0x0000045d8a1c logging::LogMessage::~LogMessage()
#2 0x00000836ee4d blink::beforeCallEnteredCallback()
#3 0x000000dbb531 v8::Function::Call()
#4 0x00000837f24b blink::(anonymous namespace)::throwStackOverflowExceptionIfNeeded()
#5 0x000008380893 blink::V8ScriptRunner::callFunction()
#6 0x00000dabfe96 test_runner::AccessibilityController::NotificationReceived()
#7 0x00000daa0858 test_runner::WebFrameTestClient::postAccessibilityEvent()
#8 0x00000dc36da4 test_runner::WebFrameTestProxy<>::postAccessibilityEvent()
#9 0x0000080f2da3 blink::ChromeClientImpl::postAccessibilityNotification()
#10 0x000009974fb4 blink::FrameView::updateScrollOffset()
#11 0x000007d44938 blink::ScrollableArea::scrollOffsetChanged()
#12 0x000007ea49d5 blink::ScrollAnimator::adjustAnimationAndSetScrollOffset()
#13 0x000007d44095 blink::ScrollableArea::setScrollOffset()
#14 0x000009a63c56 blink::RootFrameViewport::distributeScrollBetweenViewports()
#15 0x00000aae0c7f blink::ScrollAnchor::adjust()
#16 0x00000995f48a blink::FrameView::performScrollAnchoringAdjustments()
#17 0x0000090d5f6a blink::Document::updateStyleAndLayout()
#18 0x0000090d54f6 blink::Document::updateStyleAndLayoutIgnorePendingStylesheets()
#19 0x0000099f3c67 blink::LocalDOMWindow::scrollBy()
#20 0x00000856c868 blink::V8Window::scrollByMethodCallback()
#21 0x000000d812f4 v8::internal::FunctionCallbackArguments::Call()
#22 0x000000ec98a8 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#23 0x000000ec79e8 v8::internal::Builtin_Impl_HandleApiCall()
#24 0x2a75887843a2 <unknown>


Assigning to @dgozman who seems to be familiar with this stuff according to git blame. 

Cc: dgozman@chromium.org
Components: Blink>Bindings
Owner: ----
Status: Untriaged (was: Assigned)
We are calling JS in script forbidden scope here: https://cs.chromium.org/chromium/src/content/shell/test_runner/accessibility_controller.cc?rcl=7950721f084767700b62bb6e1c90ea155efae980&l=194.

Should we add AllowUserAgentScript in callFunctionEvenIfScriptDisabled? Or just in AccessibilityController? What does the bindings team think?
Marking untriaged to get respective teams attention.
I'm fine with adding AllowUserAgentScript to callFunctionEvenIfScriptDisabled, as the method name suggests.

Project Member

Comment 5 by ClusterFuzz, Mar 8 2017

ClusterFuzz has detected this issue as fixed in range 454873:455052.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5790658423160832

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_ubsan_vptr_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !ScriptForbiddenScope::isScriptForbidden() in V8PerIsolateData.cpp
  blink::beforeCallEnteredCallback
  v8::Function::Call
  
Sanitizer: undefined (UBSAN)

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_vptr_content_shell_drt&range=450496:450552
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_vptr_content_shell_drt&range=454873:455052

Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95OrszEmJ1eXktp9hMIBBJWAIhJ93ZVovu9ULhB5enDkp3aJpwUZ15ClmBkguUxiv0idji-rUBPTebPbhPvivOkhijM-KT1UcTyGfKgIUi63jdP8uWPV_Sg6b0S_GF7aPV4oxtWicMDHtDZi7IZZ9s2H_OwXKEMGGJBQ5GKribUIZypCkU7i-pdaz-UdFWao6WHOWRO3nZW0ZnoCnE6oUdsH5Ba1Hx6at-WNuIln3xRb7beAtMdd8OqmgD_4KskA0M0QBCx2o7ezFYTfWK5m-1jGA0fsx9BkoWW8SLVOz8aDcjIYmr29nkll3QvJ7Cl3GsxcNZ40XVpLXbOb-upbnf4MUGR4cZGHnYWM8nXLzxnFr6wuCeejS6iNxyAtaa04E2pJE5cpM_GorzWHkeejGRP3CNevA?testcase_id=5790658423160832


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 6 by ClusterFuzz, Mar 8 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Untriaged)
ClusterFuzz testcase 5790658423160832 is verified as fixed, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Status: Untriaged (was: Verified)
Could someone please confirm that this issue was actually fixed?
Cc: rossberg@chromium.org
Status: Available (was: Untriaged)
Owner: adithyas@chromium.org
Status: Assigned (was: Available)
Adithya, could you have a look at this? You have recent experience with ScriptForbiddenScope.
The clusterfuzz test case doesn't seem to reproduce the issue anymore, but I can't really tell what fixed it. It does make sense to add an AllowUserAgentScript inside CallFunctionEvenIfScriptDisabled though, so I'll write up a CL to do that. 
I've got a patch for that [1], but decided to close it since the crash does not happen anymore, and avoiding ScriptForbiddenScope at high level is extra risk of introducing inconsistent behavior.

[1] https://codereview.chromium.org/2728303006/
#11: Shouldn't a function called CallFunctionEvenIfScriptDisabled actually be able to run when script is disabled? (unless script being disabled is a separate concept from script being forbidden, in which case we probably shouldn't add the AllowUserAgentScript)
Apparently, those concepts are different. Script disabled is usually a global thing, while script forbidden is a temporary scoped restriction (e.g. during DOM operation).

As pointed in #c3, I too think it's logical to bypass ScriptForbiddenScope to execute script no matter what in this method, but maybe we shouldn't do that unless it's necessary?
Status: WontFix (was: Assigned)
Ok, I don't think we should add the AllowUserAgentScript then. 

I'm closing this issue.

Sign in to add a comment