Skip to content
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

Zstd-codec crashes in Node v20 and v22 due to deprecations #303

Closed
pimterry opened this issue Jul 3, 2024 · 6 comments
Closed

Zstd-codec crashes in Node v20 and v22 due to deprecations #303

pimterry opened this issue Jul 3, 2024 · 6 comments

Comments

@pimterry
Copy link
Contributor

pimterry commented Jul 3, 2024

Hi @yoshihitoh.

For quite some time, zstd-codec has printed a DeprecationWarning: Buffer() is deprecated due to security and usability issues. warning when used in modern Node.

Unfortunately due to nodejs/node#53075 this now crashes Node completely, making zstd-codec unusable in both LTS and current Node versions. A fix for that is coming, but not available yet (PR still pending), and regardless it would be good to fix this deprecation. You can reproduce this by running the tests from the js directory on the current develop branch with Node v22.3.0:

$ cd js
$ npm test

> [email protected] test
> jest

(node:2994346) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:2994347) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
 FAIL  lib/__tests__/zstd-stream.js
  ● ZstdXXXTransform › should decompress zst file

    TypeError: Cannot read properties of undefined (reading '0')

      at isInsideNodeModules (node:internal/util:521:17)
      at showFlaggedDeprecation (node:buffer:178:8)
      at new Buffer (node:buffer:266:3)
      at emval_allocator_3 (eval at craftEmvalAllocator (lib/zstd-codec-binding-wasm.js:8:951735), <anonymous>:13:11)
      at newer (lib/zstd-codec-binding-wasm.js:8:952078)
      at null.<anonymous> (wasm:/wasm/0025c056:1:4245)
      at null.<anonymous> (wasm:/wasm/0025c056:1:5896)
      at null.<anonymous> (wasm:/wasm/0025c056:1:9757)
      at null.<anonymous> (wasm:/wasm/0025c056:1:594761)
      at apply (lib/zstd-codec-binding-wasm.js:8:963066)

I can see there's also a deprecation for use of Punycode that pops up here - I haven't seen that myself at runtime, so maybe it's from the test suite, but it might be nice to clean up too alongside this if that's easy.

I'd love to get involved again and help get this fixed! Although in this case, I think that'll require some updating in the emscripten setup itself to avoid the new Buffer calls here and I'm not sure exactly how that part of the build works... Also, it looks like you're working on an upcoming v0.2.x release on another branch, but develop still hasn't included those changes, so I'm not sure what state that v0.2.x code is in (I can't see how to run any tests there, for example).

It's possible this is actually already fixed in v0.2.x, but I can't easily see how to check.

It'd be great to have this fixed, so if you can give me some pointers on where to get started on that I'm happy to dive in and help get zstd-codec working again.

@yoshihitoh
Copy link
Owner

yoshihitoh commented Jul 4, 2024

Hi @pimterry , thank you for reporting the issue.

I'd love to get involved again and help get this fixed! Although in this case, I think that'll require some updating in the emscripten setup itself to avoid the new Buffer calls here

Thanks! zstd-codec uses ArrayBuffer, not the Buffer, to interpolate data between JS and C++, and using new Buffer if I remember itcorrecly.
I'm not sure, but maybe old Emscripten version uses new Buffer for Node.js environment.
I'll take a look, but maybe on weekends.

@yoshihitoh
Copy link
Owner

Hi @pimterry, I couldn't work on this issue for a while, sorry,

The cause is here, CloneToVector invoked new Buffer() as the result.

I could solve this by Emscripten's approach, but some tasks must be done because the build system is broken.
It'll take a few more days...
image

@pimterry
Copy link
Contributor Author

Ah, that makes sense, ok. I tried to take a look myself but I didn't get very far, thanks so much for properly looking into this! 🙏

@yoshihitoh
Copy link
Owner

The deprecation warning about punycode is raised by Jest's dependencies; I think that's why you didn't see this warning at runtime.

(node:89072) [DEP0040] DeprecationWarning: The punycode module is deprecated. Please use a userland alternative instead.
at node:punycode:3:9
at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:398:7)
at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/realm:337:10)
at loadBuiltinModule (node:internal/modules/helpers:96:7)
at Function.Module._load (node:internal/modules/cjs/loader:1063:17)
at wrapModuleLoad (node:internal/modules/cjs/loader:212:19)
at Module.require (node:internal/modules/cjs/loader:1297:12)
at require (node:internal/modules/helpers:123:16)
at Object. (/Users/yoshihitoh/workspace/private/github/zstd-codec/js/node_modules/psl/index.js:5:16)
at Module._compile (node:internal/modules/cjs/loader:1460:14)

I want to fix it but I couldn't find a quick solution.
I'll address on this deprecation warning on next release.

@yoshihitoh
Copy link
Owner

@pimterry
Today I released zstd-codec 0.1.5.
Let me know if you have any problems please.

@pimterry
Copy link
Contributor Author

Amazing work, thanks @yoshihitoh! Yes, I've updated now, this issue is definitely resolved which is great, and otherwise it all seems to be working well so far 👍

Thanks for your help! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants