pvorb / clone

deeply clone arbitrary objects in javascript
https://www.npmjs.com/package/clone
MIT License
781 stars 129 forks source link

Clone breaking v8 somehow #106

Closed NicolasCaous closed 5 years ago

NicolasCaous commented 5 years ago

Error

> // contextTree is just a random object with a sequelize instance attached to it
> require('clone')(contextTree);

FATAL ERROR: v8::Object::SetInternalField() Internal field out of bounds

What I was trying to do

Inside contextTree there is a sequelize record instance. I was trying to clone it.

What I have done to try to find the root cause

I isolated the error trying to clone its properties in isolation (require('clone')(contextTree.a), require('clone')(contextTree.b), etc...) and could verify that the problem resides somewhere in the sequelize reference. However, I'm just a shitty developer without time to investigate more about its root causes. :(

Steps to reproduce

  1. A working postgres database. (don't know if it matters, but my setup is like this)
  2. Sequelize and clone installed
  3. 
    const Sequelize = require('sequelize');
    const sequelize = new Sequelize(
      'database-name',
      'user',
      'password',
      { host: 'ip', dialect: 'postgres' }
    );
    
    const Dummy = sequelize.define('dummy', {
      id: {
        allowNull: false,
        autoIncrement: true,
        primaryKey: true,
        unique: true,
        type: Sequelize.BIGINT
      }
    });
    await Dummy.sync();
    let dummy = new Dummy({id: 1});
    await dummy.save();
    require('clone')(dummy);

Conclusion

The library shouldn't be liable to what is being cloned, but I find it bizarre that it breaks v8. I don't think it was suppose to do that even while cloning adverse and complex objects. There is something really wrong that I wasn't able to pin down. I think it is worth investigating.

ENV

> console.dir(process)
process {
  version: 'v12.7.0',
  versions: {
    node: '12.7.0',
    v8: '7.5.288.22-node.16',
    uv: '1.30.1',
    zlib: '1.2.11',
    brotli: '1.0.7',
    ares: '1.15.0',
    modules: '72',
    nghttp2: '1.39.1',
    napi: '4',
    llhttp: '1.1.4',
    http_parser: '2.8.0',
    openssl: '1.1.1c',
    cldr: '35.1',
    icu: '64.2',
    tz: '2019a',
    unicode: '12.1'
  },
  arch: 'x64',
  platform: 'linux',
  ...
  config: {
    ...
    variables: {
      asan: 0,
      build_v8_with_gn: false,
      coverage: false,
      debug_nghttp2: false,
      enable_lto: false,
      enable_pgo_generate: false,
      enable_pgo_use: false,
      force_dynamic_crt: 0,
      gas_version: '2.31',
      host_arch: 'x64',
      icu_data_in: '../../deps/icu-small/source/data/in/icudt64l.dat',
      icu_endianness: 'l',
      icu_gyp_path: 'tools/icu/icu-generic.gyp',
      icu_locales: 'en,root',
      icu_path: 'deps/icu-small',
      icu_small: true,
      icu_ver_major: '64',
      is_debug: 0,
      llvm_version: 0,
      napi_build_version: '4',
      node_byteorder: 'little',
      node_code_cache: 'yes',
      node_debug_lib: false,
      node_enable_d8: false,
      node_install_npm: true,
      node_module_version: 72,
      node_no_browser_globals: false,
      node_prefix: '/usr/local',
      node_release_urlbase: '',
      node_report: true,
      node_shared: false,
      node_shared_cares: false,
      node_shared_http_parser: false,
      node_shared_libuv: false,
      node_shared_nghttp2: false,
      node_shared_openssl: false,
      node_shared_zlib: false,
      node_tag: '',
      node_target_type: 'executable',
      node_use_bundled_v8: true,
      node_use_dtrace: false,
      node_use_etw: false,
      node_use_large_pages: false,
      node_use_node_snapshot: true,
      node_use_openssl: true,
      node_use_v8_platform: true,
      node_with_ltcg: false,
      node_without_node_options: false,
      openssl_fips: '',
      openssl_is_fips: false,
      shlib_suffix: 'so.72',
      target_arch: 'x64',
      v8_enable_gdbjit: 0,
      v8_enable_i18n_support: 1,
      v8_enable_inspector: 1,
      v8_no_strict_aliasing: 1,
      v8_optimized_debug: 1,
      v8_promise_internal_field_count: 1,
      v8_random_seed: 0,
      v8_trace_maps: 0,
      v8_use_siphash: 1,
      v8_use_snapshot: 1,
      want_separate_host_toolset: 0
    }
  },
  ...
}
pvorb commented 5 years ago

Your steps to reproduce don’t work:

/home/pvo/workspace/js/clone/issue-106/test.js:18
await Dummy.sync();
^^^^^

SyntaxError: await is only valid in async function
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:616:28)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3
pvorb commented 5 years ago

I now changed your sample script to the following:

const Sequelize = require('sequelize');
const clone = require('clone');

async function test() {
    const sequelize = new Sequelize(
        'database-name',
        'user',
        'password',
        { host: 'ip', dialect: 'postgres' }
    );

    const Dummy = sequelize.define('dummy', {
        id: {
            allowNull: false,
            autoIncrement: true,
            primaryKey: true,
            unique: true,
            type: Sequelize.BIGINT
        }
    });
    await Dummy.sync();
    const dummy = new Dummy({id: 1});
    await dummy.save();
    const clonedDummy = clone(dummy);

    console.log(clonedDummy);
}

test();

This works flawlessly with Node.js v8.x, but doesn’t work with Node.js v10.16.11, failing with the following exception:

(node:18060) UnhandledPromiseRejectionWarning: TypeError: Cannot assign to read only property 'writeQueueSize' of object '#<TCP>'
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:16)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at _clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:162:18)
    at clone (/home/pvo/workspace/js/clone/issue-106/node_modules/clone/clone.js:202:10)
    at test (/home/pvo/workspace/js/clone/issue-106/test.js:24:25)
(node:18060) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:18060) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

So the root cause is clear. Sequelize opens a TCP connection, which, of course, cannot be cloned because it's a resource handle.

In order to make the object clonable, remove the reference to the TCP connection first, e.g. by deleting the _modelOptions property on the dummy object. Note that afterwards, you can no longer update and persist that object.

This code works:

const Sequelize = require('sequelize');
const clone = require('clone');

async function test() {
    const sequelize = new Sequelize(
        'clone_issue_106',
        'postgres',
        'postgres',
        {host: 'localhost', dialect: 'postgres'}
    );

    const Dummy = sequelize.define('dummy', {
        id: {
            allowNull: false,
            autoIncrement: true,
            primaryKey: true,
            unique: true,
            type: Sequelize.BIGINT
        }
    });
    await Dummy.sync();
    const dummy = new Dummy({id: 1});
    await dummy.save();

    delete dummy._modelOptions;

    const clonedDummy = clone(dummy);

    console.log(clonedDummy);
}

test();

I'm closing this, since I do not consider this a bug. Feel free to re-open if you do not agree.

pvorb commented 5 years ago

Just checked with Node v12.7.0 and got the same FATAL ERROR: v8::Object::SetInternalField() Internal field out of bounds as you did. While the error is different, the solution also works with Node v12.7.0.

NicolasCaous commented 5 years ago

@pvorb So, do you think it is an issue with clone (and therefore we should open it again) or should we investigate a little more and open a ticket elsewhere? I have never seen v8 cracking like this.

In a few days I will be able to investigate it further, but I want to know if you think it's worthwhile.

pvorb commented 5 years ago

Well, in order to clone the TCP connection, clone tries to set an internal field, which is not allowed. (This is probably because sequelize uses the pg module and pg-native internally, which is written in C++ and maybe some protection mechanism prevents clone from setting a field.)

I don't think there is an issue with Node.js, since it is calling into native code. Native objects cannot be cloned using clone, but it tries anyway because it cannot see the difference. The same is true for resources. The TCP connection from your code is both – a native object as well as a resource.

btsimonh commented 5 years ago

Just hit this same issue. clone is being used by NodeRed (i.e. I have no control over it's use or the implementation). In my case, including a ChildProcess object in a node-red message causes the process/V8 to die with FATAL ERROR: v8::Object::SetInternalField() Internal field out of bounds (node 12.10.0)

A variation of https://github.com/pvorb/clone/pull/93 could people to avoid this (kind of) error where they are not in control of the rest of the code which calls clone. I will propose a new PR.

reproduction:

var clone = require('clone');
var cp = require('child_process');
var spawn = cp.spawn;
var child = spawn('cmd', [], {});
var obj = {
  child: child
};
var copy = clone(obj);
btsimonh commented 5 years ago

PR added for extending clone, and basic crash prevention.