Skip to content

SIGILL (Illegal Instruction / Failed DCHECK) in Debug build and SIGSEGV in Release when heapdumping #18223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
STRML opened this issue Jan 18, 2018 · 5 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@STRML
Copy link
Contributor

STRML commented Jan 18, 2018

  • Version: Node 8.9.4 Debug Build
  • Platform: OSX Sierra
  • Subsystem: None/V8?

I have an application I've been unable to heapdump for some time without segfaulting. Tracking it down has been extremely frustrating as the segfault was not consistent; tests would need to be run 10+ times per configuration to confirm the segfault was removed. Switching to the Debug build (tag v8.9.4) gives me a consistent SIGILL when heapdumping:

$ ../../forks/node/out/Debug/node --inspect index.js                                                                                                                                                           23:16:49
Debugger listening on ws://127.0.0.1:9229/cfe57000-94ad-4f71-a00c-831b592f1f34
For help see https://nodejs.org/en/docs/inspector
Debugger attached.

#
# Fatal error in ../deps/v8/src/profiler/heap-snapshot-generator.cc, line 1306
# Debug check failed: constructor_or_backpointer->IsJSFunction() || constructor_or_backpointer->IsNull(map->GetIsolate()).
#
'../../forks/node/out/Debug/node…' terminated by signal SIGILL (Illegal instruction)

I eventually narrowed it down to Lodash 3.10.1 and started deleting lines to get to a minimum possible reproduction. This is as small as I could get it:

function baseLodash() {}

var baseCreate = (function() {
  function object() {}
  return function(prototype) {
    var result = new object;
    object.prototype = undefined;
  };
}());

function createCtorWrapper(Ctor) {
  return function() {
    return baseCreate(Ctor.prototype);
  };
}

baseCreate(baseLodash.prototype);

setInterval(() => {}, 1000);

Running this then heapdumping it will consistently fail the DCHECK on my machine.

To more quickly debug, install heapdump and append:

var heapdump = require('heapdump');
heapdump.writeSnapshot('./' + Date.now() + '.heapsnapshot');

which will trigger the SIGILL / DCHECK failure almost immediately.

Reproduction repository at https://github.com/STRML/lodash-heapdump-sigill.

V8 Bug at https://bugs.chromium.org/p/v8/issues/detail?id=7328

@STRML STRML changed the title SIGILL (Illegal Instruction) in Debug build and SIGSEGV in Release when heapdumping SIGILL (Illegal Instruction / Failed DCHECK) in Debug build and SIGSEGV in Release when heapdumping Jan 18, 2018
@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. confirmed-bug Issues with confirmed bugs. labels Jan 18, 2018
@addaleax
Copy link
Member

addaleax commented Jan 18, 2018

Fix is @ https://chromium-review.googlesource.com/c/v8/v8/+/874472 (edit: for the DCHECK. I couldn’t reproduce the segfault?)

kisg pushed a commit to paul99/v8mips that referenced this issue Jan 19, 2018
A map’s `constructor_or_backpointer` can be any kind of value,
because `fn.prototype = foo` sets that field to `foo` if the
latter is not a `JSReceiver`; so the `DCHECK` that is being
removed here was invalid.

Refs: nodejs/node#18223
Bug: node:18223
Change-Id: Ia6449c07bb724e515d73b162369ab36ab1d89c6b
Reviewed-on: https://chromium-review.googlesource.com/874472
Commit-Queue: Jakob Kummerow <[email protected]>
Reviewed-by: Jakob Kummerow <[email protected]>
Cr-Commit-Position: refs/heads/master@{#50735}
@STRML
Copy link
Contributor Author

STRML commented Jan 26, 2018

I'm still working on a reliable reproduction of the segfault. It's proving very elusive and may not have been related to this exact code.

Kmaschta added a commit to Kmaschta/node that referenced this issue Feb 14, 2018
Original commit message:

    [heap-profiler] remove bogus DCHECK

    A map’s `constructor_or_backpointer` can be any kind of value,
    because `fn.prototype = foo` sets that field to `foo` if the
    latter is not a `JSReceiver`; so the `DCHECK` that is being
    removed here was invalid.

    Refs: nodejs#18223
    Bug: node:18223
    Change-Id: Ia6449c07bb724e515d73b162369ab36ab1d89c6b
    Reviewed-on: https://chromium-review.googlesource.com/874472
    Commit-Queue: Jakob Kummerow <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#50735}

Refs: v8/v8@55b4879
Fixes: nodejs#18223
@STRML
Copy link
Contributor Author

STRML commented Feb 27, 2018

Backtrace of the SIGSEGV:

PID 40097 received SIGSEGV for address: 0x2
0   segfault-handler.node               0x0000000105699168 _ZL16segfault_handleriP9__siginfoPv + 280
1   libsystem_platform.dylib            0x00007fffc037bb3a _sigtramp + 26
2   ???                                 0x00007fff5fbf57f0 0x0 + 140734799763440
3   node                                0x0000000100770b36 _ZN2v88internal14SnapshotFiller14FindOrAddEntryEPvPNS0_20HeapEntriesAllocatorE + 198
4   node                                0x0000000100766939 _ZN2v88internal14V8HeapExplorer19SetContextReferenceEPNS0_10HeapObjectEiPNS0_6StringEPNS0_6ObjectEi + 57
5   node                                0x000000010076348b _ZN2v88internal14V8HeapExplorer24ExtractContextReferencesEiPNS0_7ContextE + 139
6   node                                0x00000001007717ee _ZN2v88internal14V8HeapExplorer27IterateAndExtractSinglePassIXadL_ZNS1_22ExtractReferencesPass2EiPNS0_10HeapObjectEEEEEbv + 1422
7   node                                0x000000010076769f _ZN2v88internal14V8HeapExplorer27IterateAndExtractReferencesEPNS0_14SnapshotFillerE + 335
8   node                                0x000000010076c4c8 _ZN2v88internal21HeapSnapshotGenerator16GenerateSnapshotEv + 296
9   node                                0x000000010075b35c _ZN2v88internal12HeapProfiler12TakeSnapshotEPNS_15ActivityControlEPNS_12HeapProfiler18ObjectNameResolverE + 92
10  node                                0x00000001001a7457 _ZN12v8_inspector23V8HeapProfilerAgentImpl16takeHeapSnapshotENS_8protocol5MaybeIbEE + 263
11  node                                0x000000010015e1cf _ZN12v8_inspector8protocol12HeapProfiler14DispatcherImpl16takeHeapSnapshotEiNSt3__110unique_ptrINS0_15DictionaryValueENS3_14default_deleteIS5_EEEEPNS0_12ErrorSupportE + 447
12  node                                0x000000010015d920 _ZN12v8_inspector8protocol12HeapProfiler14DispatcherImpl8dispatchEiRKNS_8String16ENSt3__110unique_ptrINS0_15DictionaryValueENS6_14default_deleteIS8_EEEE + 128
13  node                                0x0000000100143f57 _ZN12v8_inspector8protocol14UberDispatcher8dispatchENSt3__110unique_ptrINS0_5ValueENS2_14default_deleteIS4_EEEE + 1879
14  node                                0x00000001001b0889 _ZN12v8_inspector22V8InspectorSessionImpl23dispatchProtocolMessageERKNS_10StringViewE + 41
15  node                                0x00000001009fcb57 _ZN4node9inspector11InspectorIo16DispatchMessagesEv + 297
16  node                                0x00000001009c8407 _ZN4nodeL17RunForegroundTaskEPN2v84TaskE + 93
17  node                                0x00000001009c8364 _ZN4node12NodePlatform28FlushForegroundTasksInternalEv + 244
18  node                                0x0000000100aff7d6 uv__async_io + 317
19  node                                0x0000000100b0f83c uv__io_poll + 1934
20  node                                0x0000000100affc57 uv_run + 321
21  node                                0x000000010098965b _ZN4node5StartEPN2v87IsolateEPNS_11IsolateDataEiPKPKciS8_ + 797
22  node                                0x00000001009836a5 _ZN4node5StartEP9uv_loop_siPKPKciS5_ + 454
23  node                                0x0000000100982afc _ZN4node5StartEiPPc + 469
24  node                                0x0000000100001834 start + 52

addaleax added a commit to addaleax/node that referenced this issue Apr 12, 2018
Original commit message:

    [heap-profiler] remove bogus DCHECK

    A map’s `constructor_or_backpointer` can be any kind of value,
    because `fn.prototype = foo` sets that field to `foo` if the
    latter is not a `JSReceiver`; so the `DCHECK` that is being
    removed here was invalid.

    Refs: nodejs#18223
    Bug: node:18223
    Change-Id: Ia6449c07bb724e515d73b162369ab36ab1d89c6b
    Reviewed-on: https://chromium-review.googlesource.com/874472
    Commit-Queue: Jakob Kummerow <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#50735}

Refs: v8/v8@55b4879
Fixes: nodejs#18223
@gireeshpunathil
Copy link
Member

Just wondering In the absence of a test case, how do we validate that this is fixed? @STRML - can you see if the master solves this issue? /cc @addaleax

targos pushed a commit that referenced this issue Jun 6, 2018
Original commit message:

    [heap-profiler] remove bogus DCHECK

    A map’s `constructor_or_backpointer` can be any kind of value,
    because `fn.prototype = foo` sets that field to `foo` if the
    latter is not a `JSReceiver`; so the `DCHECK` that is being
    removed here was invalid.

    Refs: #18223
    Bug: node:18223
    Change-Id: Ia6449c07bb724e515d73b162369ab36ab1d89c6b
    Reviewed-on: https://chromium-review.googlesource.com/874472
    Commit-Queue: Jakob Kummerow <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#50735}

PR-URL: #18339
Fixes: #18223
Refs: v8/v8@55b4879
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 25, 2018

This was fixed by bce5d45 but the issue was never closed. (Comment or re-open if I'm mistaken!)

@Trott Trott closed this as completed Nov 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants