Internal node assertion caused by js copy mechanism #55302
Comments
artur-ma
changed the title
|
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? |
|
CC @nodejs/undici |
|
@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 |
|
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) |
|
Because this can't be reproduced, and there's no minimal reproduction, I've closed it. I'll reopen if you give more info. |
|
@RedYetiDev wdym cant be reproduced? I gave exact reproduction steps.. undici is part of nodejs project, the fast-copy is copying the object |
|
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? |
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
added
confirmed-bug
|
I'm able to reproduce now, thank you :-): Repro Aconst 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();Repro Bconst 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(); |
|
This is not a bug, but incorrect use of Node.js internals. |
|
@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) |
|
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 |
|
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. |
|
let me ask such question, pino-pretty uses same lib (fast-copy) 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) |
|
Well, the assertion (AFAIK) can't be caught because it's thrown from C++land, as the |
|
I would expect any library that uses fast-copy to randomly crash. @mcollina wdyt? |
+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'));
}); |
Yes I understand that, my "proposal" if its possible, to "redeclare" the constructor if possible on JS side
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()
}
} |
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. |
Version
22.9.0
Platform
Subsystem
No response
What steps will reproduce the bug?
Run in node cli:
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
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
The text was updated successfully, but these errors were encountered: