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

Issue metadata

Status: Verified
Owner:
Last visit 25 days ago
Closed: Feb 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

DCHECK failure in op->IsAnyLocationOperand() in instruction.h

Project Member Reported by ClusterFuzz, Jan 15 2018

Issue description

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  op->IsAnyLocationOperand() in instruction.h
  cast
  ToRegister
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=50568:50569

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jan 15 2018

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.
Project Member

Comment 2 by ClusterFuzz, Jan 15 2018

Cc: pierre.l...@arm.com
Labels: Test-Predator-Auto-CC
Automatically adding ccs based on suspected regression changelists:

[turbofan] Lower NumberConstant nodes to IntPtrConstant. by pierre.langlois@arm.com - https://chromium.googlesource.com/v8/v8/+/7ac10da795aa55eaf5217864972672e4d22f2709

If this is incorrect, please apply the Test-Predator-Wrong-CLs label.

Comment 3 by pierre.l...@arm.com, Jan 15 2018

Hello,

I'm afraid I do not have permissions to see the report. Do you have instructions to reproduce the issue?

Thanks,
Pierre
Cc: bmeu...@chromium.org
Owner: jarin@chromium.org
Status: Assigned (was: Untriaged)
Local bisect confirmed your CL. Assigning to Jaro for now (owner must be a project member), CC'ing Benedikt.

Minimized Reproducer:

function assertEquals(expected, found) {
  found.length !== expected.length;
}
assertEquals([], [])
assertEquals("a", "a");
assertEquals([], []);
function f() {
  assertEquals(0, undefined);
}
try {
  f();
} catch (e) {
}
%OptimizeFunctionOnNextCall(f);
f();

Labels: Pri-1

Comment 6 by pierre.l...@arm.com, Jan 16 2018

Thanks for the reproducer, I can confirm the crash on x64 with "is_asan = true". Let's see if I can find what the problem is.
Project Member

Comment 7 by ClusterFuzz, Jan 16 2018

Labels: OS-Mac

Comment 8 by pierre.l...@arm.com, Jan 16 2018

Hello,

I've been investigating this today so here is some analysis.

When f fails to compile, assertEquals is inlined and it appears that
its arguments, expected and found, were speculated to be tagged
pointers. As a result, we end up with a CheckHeapObject on a
NumberConstant(0), this is guaranteed to deopt.

Was this a scenario you were aware of?

Now to the actual crash. We have the following bits of schedule for f:

~~~
  // snip
  77: Int64Constant[0]  <--- This used to be NumberConstant[0] before this patch.
  // snip
--- BLOCK B3 <- B2, B1 ---
  // snip

  // Deopt if Int64Contant[0] is a Smi.
  99: Int64Constant[1]
  100: Word64And(77, 99)
  101: Word64Equal(100, 77)
  102: DeoptimizeIf[Eager:Smi](101, 42, 38, 38)

  // Up until now, all code is dead since we know 0 is a Smi.
  // Load map.
  137: Int64Constant[-1]
  104: HeapConstant[0x7ec12c702481 <Map(HOLEY_ELEMENTS)>]
  103: Load[kRepTaggedPointer|kTypeAny](77, 137, 102, 102)
  105: Word64Equal(103, 104)
  125: Int64Constant[32]
  107: Branch[None](105, 102) -> B5, B4
--- BLOCK B4 <- B3 ---
  109: IfFalse(107)

  // Deopt if the map does not match.
  122: HeapConstant[0x7ea59c002751 <Map(PACKED_SMI_ELEMENTS)>]
  121: Load[kRepTaggedPointer|kTypeAny](77, 137, 103, 109)
  123: Word64Equal(121, 122)
  124: DeoptimizeUnless[Eager:WrongMap](123, 42, 121, 109)

  // Load property and tag it.
  139: Int64Constant[23]
  63: Load[kRepTaggedSigned|kTypeInt32](77, 139, 124, 124) ;; Cannot compile this.
  126: Word64Sar(63, 125)                                  ;;
  127: TruncateInt64ToInt32(126)                           ;;
  Goto -> B6
--- BLOCK B5 <- B3 ---
~~~

The issue appears to be with compiling the last load+tag operation. We generate:

~~~
v10(R) = X64Movl : MR #27
~~~

instead of:

~~~
v9(R) = X64Movl : MRI v7(R) #27
~~~

And the former triggers an assertion because we expect to see a base
register and not just an immediate. Also, the address mode doesn't
look right either.

Looking through the source code led me to
https://chromium.googlesource.com/v8/v8/+/master/src/compiler/x64/instruction-selector-x64.cc#101
were we have a special handling of Int64Constant[0]. The patch which
triggers the issue replaced NumberConstant[0] with Int64Constant[0] so
this must be related. Note that this only happens when this is called
from VisitTruncateInt64ToInt32 where we try to load and tag at the
same time.

I hope all this makes sense and helps. I tried to see if I could fix
it myself but I'm afraid I'm a little lost in the x64 selector as I'm
not familiar with x64 architecture at all.

Thanks,
Pierre

Comment 9 by jarin@chromium.org, Jan 17 2018

Cc: jarin@chromium.org
Owner: danno@chromium.org
This looks like some bug in the address mode matching. Danno, could you have a look, please?
Labels: Security_Impact-Head
Labels: M-65
Project Member

Comment 12 by sheriffbot@chromium.org, Jan 21 2018

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

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

Comment 13 by danno@chromium.org, Jan 24 2018

I was also able to reproduce on x64, but I am no longer sure that this is an instruction selection problem. Examining the schedule before the crash, I get the following:

  ...
  77: Int64Constant[0]
  ...
  137: Int64Constant[-1]
  ...
  103: Load[kRepTaggedPointer|kTypeAny](77, 137, 102, 102)
  ...

This is a load from the immediate address -1, which doesn't seem right both because the address is weird and because both parameters to the load are immediate values with the first one being zero (rather than the second).

Hi danno,

I was also unclear about this. It seems the bigger issue with this test case is that the function `f` is inlining `assertEqual` while passing it values that are at odds with the feedback collected for it. `assertEqual` expects tagged pointers but we are passing the Smi value 0. So as a result of using %OptimizeFunctionOnNextCall, we end up compiling `f` while knowing for sure that it will be deoptimized. And we generate dead code as a result:

~~~
if (IsTaggedPointer(0)) {
  map = Load(0, -1);
} else {
  Deopt(kIsSmi);
}
~~~

Technically, the problematic load is dominated by the tagged pointer check so we should be safe. Which then made me think maybe it's OK to compile this and then the problem would be with the x64 selector getting confused when it shouldn't.

Also, is it correct that this strange state we end up in here would not normally occur? Here we have use %OptimizedFunctionOnNextCall to force TurboFan to compile `f` when it shouldn't have.

Does this make sense?

Finally, I noticed cases similar to this in CSA code, where we pass NumberConstant(0) to a method that can handle both Smis and HeapNumbers (see https://chromium.googlesource.com/v8/v8/+/master/src/builtins/builtins-array-gen.cc#1397). This generates similar code where we statically know the result of a Smi check but still generate code for both outcomes. We generate "load at address 0 - offset" code in these cases too. We have an internal task to maybe look into optimizing those out one day but that's another topic :-).

Comment 15 by jarin@chromium.org, Jan 25 2018

It sometimes does happen that static type and type feedback disagree in the compiler. Typically, when we inline a function that has collected feedback somewhere else (from some other call site). If that happens then the compiler might see nonsensical operations, but that's fine because the code is unreachable. We just need to survive the compilation.

We already had this problem in other places (@tebbi has great experience with fixing such bugs).
Project Member

Comment 16 by sheriffbot@chromium.org, Jan 25 2018

Labels: -Security_Impact-Head Security_Impact-Beta

Comment 17 by jarin@chromium.org, Jan 26 2018

Cc: neis@chromium.org

Comment 18 by jarin@chromium.org, Jan 26 2018

Cc: machenb...@chromium.org
See also V8 issue https://crbug.com/v8/7362. Maybe duplicate? It runs into the same dcheck with the gc fuzzer. Repros reliably just with optdebug.
Project Member

Comment 20 by bugdroid1@chromium.org, Feb 1

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

commit 9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02
Author: Daniel Clifford <danno@chromium.org>
Date: Thu Feb 01 14:44:19 2018

Fix bug in x64 immediate operand handling for smi-converting loads

Bug:  chromium:802060 
Change-Id: I032930af26f7eab8d5d3469ad273bdcdff85b045
Reviewed-on: https://chromium-review.googlesource.com/897723
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Daniel Clifford <danno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51035}
[modify] https://crrev.com/9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02/src/compiler/x64/instruction-selector-x64.cc
[add] https://crrev.com/9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02/test/mjsunit/regress/regress-802060.js

Project Member

Comment 21 by ClusterFuzz, Feb 2

ClusterFuzz has detected this issue as fixed in range 51034:51035.

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  op->IsAnyLocationOperand() in instruction.h
  cast
  ToRegister
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=50568:50569
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=51034:51035

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

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 22 by ClusterFuzz, Feb 2

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 4563349668102144 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 23 by sheriffbot@chromium.org, Feb 8

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

Comment 24 by sheriffbot@chromium.org, Feb 8

Labels: Merge-Request-65
Project Member

Comment 25 by sheriffbot@chromium.org, Feb 9

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: M65 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), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
[Bulk Edit]

+awhalley@ (Security TPM) for M65 merge review
govind@ - good for 65.
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 branch 3325 based on comment #27. Please merge ASAP. Thank you.
Project Member

Comment 29 by bugdroid1@chromium.org, Feb 9

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

commit cc2e87e4bb15bff9423336db29131ab7afa86e99
Author: Daniel Clifford <danno@chromium.org>
Date: Fri Feb 09 11:14:27 2018

Merged: Fix bug in x64 immediate operand handling for smi-converting loads

NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Bug:  chromium:802060 
Change-Id: I032930af26f7eab8d5d3469ad273bdcdff85b045
Reviewed-on: https://chromium-review.googlesource.com/897723
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Daniel Clifford <danno@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#51035}(cherry picked from commit 9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02)
Reviewed-on: https://chromium-review.googlesource.com/910928
Cr-Commit-Position: refs/branch-heads/6.5@{#35}
Cr-Branched-From: 73c55f57fe8506011ff854b15026ca765b669700-refs/heads/6.5.254@{#1}
Cr-Branched-From: 594a1a0b6e551397cfdf50870f6230da34db2dc8-refs/heads/master@{#50664}
[modify] https://crrev.com/cc2e87e4bb15bff9423336db29131ab7afa86e99/src/compiler/x64/instruction-selector-x64.cc
[add] https://crrev.com/cc2e87e4bb15bff9423336db29131ab7afa86e99/test/mjsunit/regress/regress-802060.js

Labels: -Merge-Approved-65
Per comment #29, this is already merged to M65.
Labels: -ReleaseBlock-Stable
Project Member

Comment 32 by sheriffbot@chromium.org, Mar 27

Labels: -Security_Impact-Beta Security_Impact-Stable
Project Member

Comment 33 by sheriffbot@chromium.org, May 11

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