Internal node assertion caused by js copy mechanism

[artur-ma]

Version

22.9.0

Platform

image node:22.9.0-bullseye-slim

Subsystem

No response

What steps will reproduce the bug?

Run in node cli:

const undici = require('undici')
const { default: fastCopy } = require('fast-copy')
const a = new undici.Agent()
a.request({ method: 'GET', origin: 'https://google.com', path: '/' }).then(r => r.body.text())
fastCopy(a)

How often does it reproduce? Is there a required condition?

everytime

What is the expected behavior? Why is that the expected behavior?

Have meaningful protection and error message on JS level

What do you see instead?

Process crash

│  node[80]: static void node::TCPWrap::New(const v8::FunctionCallbackInfo<v8::Value>&) at ../src/tcp_wrap.cc:155  
│   #  Assertion failed: args[0]->IsInt32()                                                                                                                                                                                                   
│                                                                                                                                                                                                                                             │
│ ----- Native stack trace -----                                                                                                                                                                                                                                                                                                                                                                                                                                                       
│  1: 0xf462ec node::Assert(node::AssertionInfo const&) [node]                                                                                                                                                                                
│  2: 0x1088d7c node::TCPWrap::New(v8::FunctionCallbackInfo<v8::Value> const&) [node]                                                                                                                                                         
│  3: 0x1239b24  [node]                                                                                                                                                                                                                       
│  4: 0x1239dcc v8::internal::Builtin_HandleApiConstruct(int, unsigned long*, v8::internal::Isolate*) [node]                                                                                                                                  
│  5: 0x1cfb8f4  [node]                                                                                                                                                                                                                                                                                                                                                                                                                                                                 │
│ ----- JavaScript stack trace -----                                                                                                                                                                                                                                                                                                                                                                                                                                                  
│ 1: getCleanClone (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:52:20)                                                                                                                                                                
│ 2: copyObjectLooseModern (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:214:17)                                                                                                                                                       
│ 3: copier (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:371:20)                                                                                                                                                                      
│ 4: copyObjectLooseModern (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:226:35)                                                                                                                                                       
│ 5: copier (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:371:20)                                                                                                                                                                      
│ 6: copyObjectLooseModern (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:226:35)                                                                                                                                                       
│ 7: copier (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:371:20)                                                                                                                                                                      
│ 8: copyArrayLoose (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:147:30)                                                                                                                                                              
│ 9: copier (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:367:20)                                                                                                                                                                      
│ 10: copyObjectLooseModern (/home/app/node_modules/fast-copy/dist/cjs/index.cjs:226:35)     

Additional information

Using pure js library, without native code manipulation causes internal nodejs error and process crash

here is the library https://github.com/planttheidea/fast-copy

[RedYetiDev]

Can you reproduce without the use of this external library? If not, can you report the issue to them and find a minimal reproduction

Can you reproduce without Undici?

[RedYetiDev]

CC @nodejs/undici

[artur-ma]

@RedYetiDev The issues is here because pure js code can cause total process crash, the library is using different techniques to copy objects, but it should not cause process crash if they are not touching internal code (C++), and they are not using process.binding

[artur-ma]

This issue reported multiple times, but there was always an assumption that some dependency is manipulating internals

https://github.com/nodejs/node/issues/46093 https://github.com/nodejs/node/issues/18389

Which is not true, this bug can occure even without touching internals

[RedYetiDev]

I can't reproduce the issue at all...:

const undici = require('undici')
const { default: fastCopy } = require('fast-copy')
const a = new undici.Agent()
a.request({ method: 'GET', origin: 'https://google.com', path: '/' }).then(r => r.body.text())
fastCopy(a)

[RedYetiDev]

Because this can't be reproduced, and there's no minimal reproduction, I've closed it. I'll reopen if you give more info.

[artur-ma]

@RedYetiDev wdym cant be reproduced? I gave exact reproduction steps..

undici is part of nodejs project, the fast-copy is copying the object

[RedYetiDev]

I can't reproduce with the steps you gave. Try reporting the issue to Undici, they may be able to help better.

Undici has some CPP bindings, so it might be an Undici issue?

[artur-ma]

@RedYetiDev

const net = require('net')

const socket = net.Socket()
socket.connect('google.com', 443)

for (const s of Object.getOwnPropertySymbols(socket)) {
  if (socket[s]?.constructor) {
    socket[s]?.constructor()
  }
}

Here, no dependencies...

[RedYetiDev]

I'm able to reproduce now, thank you :-):

Repro A

const net = require('net')

const socket = net.Socket();
socket.connect('google.com', 443);
const kHandle = Object.getOwnPropertySymbols(socket).find((s) => s.description === 'kHandle');
new socket[kHandle].constructor();

  #  node[90025]: static void node::PipeWrap::New(const FunctionCallbackInfo<Value> &) at ../src/pipe_wrap.cc:123
  #  Assertion failed: args[0]->IsInt32()

Repro B

const net = require('net')

const socket = net.Socket();
socket.connect('google.com', 443);
const kHandle = Object.getOwnPropertySymbols(socket).find((s) => s.description === 'kHandle');
socket[kHandle].constructor();

#  node[90067]: static void node::PipeWrap::New(const FunctionCallbackInfo<Value> &) at ../src/pipe_wrap.cc:122
#  Assertion failed: args.IsConstructCall()

[targos]

This is not a bug, but incorrect use of Node.js internals.

[artur-ma]

@targos As I mentioned before, I would expect it not to crash the whole process but to have meaningfull error and normal stack trace (if possible)

[artur-ma]

When I go over an object recursievly which by coincidence has socket instace reference, I do not completley agree this is "incorrect use of Node.js internals"

This is what "fast-copy" doing actually

[ronag]

I think when you are accessing node internals it's undefined behavior. There is no way we can provide meaningful behavior when things are accessed in a way that is not intended. If anything I would argue that the fix here is that the handle should not be available at all.

[artur-ma]

let me ask such question, pino-pretty uses same lib (fast-copy) https://github.com/pinojs/pino-pretty/blob/ba1e8448f1364f7d14e7d88c9a97af48de7128dd/lib/utils/filter-log.js#L30

if i will do

const pino = require('pino')
const pretty = require('pino-pretty')
const logger = pino(pretty())
const fastify = require('fastify')

const server = fastify()

server.get(async (request) => {
  logger.info(request)
})

Do u expect the whole nodejs process to crash because request object has reference to Socket? (in this case it will not happen, because by luck pino-pretty gets already strigified data)

[RedYetiDev]

Well, the assertion (AFAIK) can't be caught because it's thrown from C++land, as the Pipe (the class that causes the assertion) is from PipeWrap, a native binding.

[ronag]

I would expect any library that uses fast-copy to randomly crash. @mcollina wdyt?

[ronag]

@targos: If anything I could agree that we should move from using private symbols to using private properties in order to avoid these kinds of undefined behaviors. It's not totally unreasonable to expect that a javascript program has only defined behaviors. @aduh95 maybe also has an opinion?

[cjihrig]

we should move from using private symbols to using private properties in order to avoid these kinds of undefined behavior

+1. I believe that would fix this issue as well:

'use strict';
const { AsyncLocalStorage } = require('node:async_hooks');
const { deepEqual } = require('node:assert/strict');
const asyncLocalStorage = new AsyncLocalStorage();

asyncLocalStorage.run({}, () => {
  deepEqual(Promise.resolve('foo'), Promise.resolve('foo'));
});

[artur-ma]

Well, the assertion (AFAIK) can't be caught because it's thrown from C++land, as the Pipe (the class that causes the assertion) is from PipeWrap, a native binding.

Yes I understand that, my "proposal" if its possible, to "redeclare" the constructor if possible on JS side because the constructors both of TCP class and Pipe class are called from native code as far as I understand, no one calling it from JS code, so maybe we can redeclare it on JS side that will throw or something like that?

If anything I could agree that we should move from using private symbols to using private properties in order to avoid these kinds of undefined behavior.

IMO That would be even better I guess, since as u can see in my repro code, only public Nodejs API were used to cause it.

const net = require('net')
const socket = net.Socket()
socket.connect('google.com', 443)

for (const s of Object.getOwnPropertySymbols(socket)) {
  if (socket[s]?.constructor) {
    socket[s]?.constructor()
  }
}

[mcollina]

I would expect any library that uses fast-copy to randomly crash. @mcollina wdyt?

Not really, it works very well for the supported use case pino-pretty - I'm fairly unsure how they where able to get that crash. There is no supported way in pino to get there.

---

In other terms, most native objects in Node would do very bad things if cloned. Don't do it.

[ravin00]

I don't think using pure js library, without native code manipulation causes internal node.js error or process crashes

[joyeecheung]

It's the use of Object.getOwnPropertySymbols that exposes internals. Symbol properties keyed by unexposed symbols should be considered internal, it's more or less okay if you are just accessing it, but trying to invoke an internal property's constructor even though you don't know how it's supposed to be invoked is dangerous business. A crash is actually better compared to letting the constructor do any random things to the process with side effects. The use case mentioned could've hit any JS land objects that aren't supposed to get its constructor called incorrectly.

class DontConstructMeExternally {
  constructor(arg) {
    if (isNotExpected(arg)) {
      process.abort();  // Or do any evil things here if external code try to abuse it.
    }
  } 
}