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

Issue 801627 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Security: V8: JIT: Type confusion in NodeProperties::InferReceiverMaps

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

Issue description

https://cs.chromium.org/chromium/src/v8/src/compiler/node-properties.cc?rcl=df84e87191022bf6914f9570069908f10b303245&l=416

Here's a snippet of NodeProperties::InferReceiverMaps.
      case IrOpcode::kJSCreate: {
        if (IsSame(receiver, effect)) {
          HeapObjectMatcher mtarget(GetValueInput(effect, 0));
          HeapObjectMatcher mnewtarget(GetValueInput(effect, 1));
          if (mtarget.HasValue() && mnewtarget.HasValue()) {
            Handle<JSFunction> original_constructor =
                Handle<JSFunction>::cast(mnewtarget.Value());

            if (original_constructor->has_initial_map()) {
              Handle<Map> initial_map(original_constructor->initial_map());
              if (initial_map->constructor_or_backpointer() ==
                  *mtarget.Value()) {
                *maps_return = ZoneHandleSet<Map>(initial_map);
                return result;
              }
            }
          }
          // We reached the allocation of the {receiver}.
          return kNoReceiverMaps;
        }
        break;
      }

"mnewtarget" is expected to be a constructor which also can be of type JSBoundFunction. But "mnewtarget" is always cast to JSFunction leading to type confusion.

The PoC seems not to crash in release mode.

Debug mode log:
#
# Fatal error in ../../src/objects-inl.h, line 566
# Check failed: !v8::internal::FLAG_enable_slow_asserts || (object->IsJSFunction()).
#

==== C stack trace ===============================

    /v8/out.gn/x64.debug/./libv8_libbase.so(v8::base::debug::StackTrace::StackTrace()+0x1e) [0x7f4623e1043e]
    /v8/out.gn/x64.debug/./libv8_libplatform.so(+0x30907) [0x7f4623db3907]
    /v8/out.gn/x64.debug/./libv8_libbase.so(V8_Fatal(char const*, int, char const*, ...)+0x1bd) [0x7f4623df876d]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::JSFunction::cast(v8::internal::Object*)+0x64) [0x7f46226584a4]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::Handle<v8::internal::JSFunction> const v8::internal::Handle<v8::internal::JSFunction>::cast<v8::internal::JSFunction>(v8::internal::Handle<v8::internal::JSFunction>)+0x23) [0x7f4622651173]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::NodeProperties::InferReceiverMaps(v8::internal::compiler::Node*, v8::internal::compiler::Node*, v8::internal::ZoneHandleSet<v8::internal::Map>*)+0x435) [0x7f4622c24a75]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::JSNativeContextSpecialization::InferReceiverMaps(v8::internal::compiler::Node*, v8::internal::compiler::Node*, std::__1::vector<v8::internal::Handle<v8::internal::Map>, std::__1::allocator<v8::internal::Handle<v8::internal::Map> > >*)+0x50) [0x7f4622b8b820]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::JSNativeContextSpecialization::ExtractReceiverMaps(v8::internal::compiler::Node*, v8::internal::compiler::Node*, v8::internal::FeedbackNexus const&, std::__1::vector<v8::internal::Handle<v8::internal::Map>, std::__1::allocator<v8::internal::Handle<v8::internal::Map> > >*)+0x202) [0x7f4622b82632]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::JSNativeContextSpecialization::ReduceNamedAccessFromNexus(v8::internal::compiler::Node*, v8::internal::compiler::Node*, v8::internal::FeedbackNexus const&, v8::internal::Handle<v8::internal::Name>, v8::internal::compiler::AccessMode)+0x2e6) [0x7f4622b822b6]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::JSNativeContextSpecialization::ReduceJSStoreNamed(v8::internal::compiler::Node*)+0x298) [0x7f4622b7c2c8]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::JSNativeContextSpecialization::Reduce(v8::internal::compiler::Node*)+0x11f) [0x7f4622b78f7f]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::GraphReducer::Reduce(v8::internal::compiler::Node*)+0x285) [0x7f4622ad8c55]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::GraphReducer::ReduceTop()+0x44f) [0x7f4622ad874f]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::GraphReducer::ReduceNode(v8::internal::compiler::Node*)+0x1bc) [0x7f4622ad7cfc]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::GraphReducer::ReduceGraph()+0x2d) [0x7f4622ad89bd]
    /v8/out.gn/x64.debug/./libv8.so(v8::internal::compiler::InliningPhase::Run(v8::internal::compiler::PipelineData*, v8::internal::Zone*)+0x58a) [0x7f4622c46e2a]

PoC:
// Flags: --allow-natives-syntax --enable_slow_asserts

class Base {
    constructor() {
        this.x = 1;
    }
}

class Derived extends Base {
    constructor() {
        // JSCreate emitted I guess.
        super();
    }
}

let bound = Object.bind();
Reflect.construct(Derived, [], bound);  // Feed a bound function as new.target to the profiler, so HeapObjectMatcher can find it.

%OptimizeFunctionOnNextCall(Derived);

new Derived();




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.

 
 
Labels: Arch-All
Owner: bmeu...@chromium.org
Status: Assigned (was: Unconfirmed)
Great catch, thanks! I'll take a look.
Project Member

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

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

commit e272a2f722422651cf5bbbe0168702ee5d38cfe8
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Jan 15 06:56:47 2018

[turbofan] Fix type confusion in NodeProperties::InferReceiverMaps.

For JSCreate nodes with constant inputs we cannot simply assume that the
new.target input is a JSFunction, since it can essentially be any
JSReceiver that is a constructor, i.e. it can also be a JSBoundFunction.

Bug:  chromium:801627 
Change-Id: Ia37bf9c0a751e4665e1167a3771fbe166473c979
Reviewed-on: https://chromium-review.googlesource.com/866493
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50563}
[modify] https://crrev.com/e272a2f722422651cf5bbbe0168702ee5d38cfe8/src/compiler/node-properties.cc
[add] https://crrev.com/e272a2f722422651cf5bbbe0168702ee5d38cfe8/test/mjsunit/regress/regress-crbug-801627.js

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

Comment 4 by sheriffbot@chromium.org, Jan 15 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: We are only 7 days from stable.
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
Project Member

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

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

Comment 6 by gov...@chromium.org, Jan 16 2018

Pls apply appropriate OSs label.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Cc: awhalley@chromium.org
+awhalley@
Labels: -Merge-Review-64 Merge-Approved-64
Please merge this today before 3:00PM PST
Ping!
Sorry, missed the notification. Will merge now.
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 19 2018

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

commit 746005554cf2fff6370399dd62cbd968736de230
Author: Benedikt Meurer <bmeurer@google.com>
Date: Fri Jan 19 07:33:19 2018

Merged: [turbofan] Fix type confusion in NodeProperties::InferReceiverMaps.

Revision: e272a2f722422651cf5bbbe0168702ee5d38cfe8

BUG= chromium:801627 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=yangguo@chromium.org

Change-Id: Ic987c9f76f2c2974d107f2bb9b1ee5988bd50354
Reviewed-on: https://chromium-review.googlesource.com/874371
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.4@{#76}
Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1}
Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724}
[modify] https://crrev.com/746005554cf2fff6370399dd62cbd968736de230/src/compiler/node-properties.cc
[add] https://crrev.com/746005554cf2fff6370399dd62cbd968736de230/test/mjsunit/regress/regress-crbug-801627.js

Comment 14 by cmasso@google.com, Jan 19 2018

Labels: -Hotlist-Merge-Review -Merge-Approved-64
Cc: hablich@chromium.org
Is this need a merge to M65? If yes, pls request a merge. Thank you.

Comment 16 by habl...@google.com, Jan 24 2018

This is already on 65
Project Member

Comment 17 by sheriffbot@chromium.org, Apr 23 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

Sign in to add a comment