zlib: revert back to Functions #13374

jasnell · 2017-06-01T18:27:23Z

Using ES6 Classes broke userland code. Revert back to functions.
This is an alternative to #13370

Technically this would be semver-major, but it fixes a regression caused by a semver-major in 8.x, so it would be semver-patch.

Fixes: #13358
Refs: #13370

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

zlib

refack

Maybe take the test from #13370 https://github.com/nodejs/node/pull/13370/files#diff-01422010a61e75fcb72efe054d70972b

jasnell · 2017-06-01T19:01:13Z

heh... was just pushing that up @refack :-)

jasnell · 2017-06-01T19:05:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8407/

jasnell · 2017-06-01T19:06:13Z

Btw, I plan to do a quick 8.0.x or 8.1.x release on Tuesday of next week that'll hopefully include this fix.

mscdex · 2017-06-01T19:15:02Z

lib/zlib.js

-    opts = opts || {};
-    super(opts);
+function Zlib(opts, mode) {
+  if (!(this instanceof Zlib))


I don't think this is needed since Zlib is not explicitly exported and is used only as a base "class" inside this module. Removing it will also help performance-wise.

watilde · 2017-06-01T19:31:29Z

lib/zlib.js


-  get _closed() {
+Object.defineProperty(Zlib.prototype, '_closed', {
+  get() {
    return !this._handle;
  }


I think we had these two descriptors at here previously.

configurable: true, enumerable: true

Using ES6 Classes broke userland code. Revert back to functions. Fixes: nodejs#13358 Refs: nodejs#13370

jasnell · 2017-06-01T20:27:24Z

Updated to address nits

mcollina

LGTM

Using ES6 Classes broke userland code. Revert back to functions. PR-URL: #13374 Fixes: #13358 Ref: #13370 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

jasnell · 2017-06-05T17:59:11Z

Landed in 7024c5a

Using ES6 Classes broke userland code. Revert back to functions. PR-URL: #13374 Fixes: #13358 Ref: #13370 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](nodejs@135f4e6643)] [nodejs#13367](nodejs#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](nodejs@968596ec77)] [nodejs#13306](nodejs#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](nodejs@ffa7debd7a)] [nodejs#13487](nodejs#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](nodejs@6e0eccd7a1)] [nodejs#13316](nodejs#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](nodejs@c756efb25a)] [nodejs#13173](nodejs#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](nodejs@cc6ec2fb27)] [nodejs#5025](nodejs#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](nodejs@6aeb555cc4)] [nodejs#13374](nodejs#13374).

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](135f4e6643)] [#13367](#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](968596ec77)] [#13306](#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](ffa7debd7a)] [#13487](#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](6e0eccd7a1)] [#13316](#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](c756efb25a)] [#13173](#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](cc6ec2fb27)] [#5025](#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](6aeb555cc4)] [#13374](#13374).

* src: use explicit imports Replace `using namespace lldb` with explicit `using lldb::<name>` imports. * test: fix scan-test.js with node >= 8.1.0 The object change that commit b73e042 ("src,test: support node.js >= 8") from April addressed has been reverted again in 8.1.0. Update the test. Refs: nodejs/node#13374 * src: print builtins and unnamed stack frames Previously, `v8 bt` would exclude frames that didn't map to a C++ symbol or a JS stack frame. llnode does not currently know how to identify the stack frames of V8 builtins so those were omitted as well. This commit makes those stack frames visible and introduces a heuristic (in lldb >= 3.9) where frames whose PC is inside a WX memory segment are assumed to belong to V8 builtins. Fixes: #99 * fixup! SBMemoryRegionInfo is lldb >= 3.9 Fix: #99 PR-URL: #104 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

This commit introduces a mutex field on the ManagedEVPPKey class intended to be used when multiple threads require access to an OpenSSL EVP_PKEY object. The motivation for this came from the work being done to upgrade Node.js to OpenSSL 3.0. OpenSSL objects, like EVP_PKEY, are not thread safe (see refs for details). In versions prior to OpenSSL 3.0 this was not noticeable and did not cause any issues (like incorrect logic or crashes), but with OpenSSL 3.0 this does cause issues if access to an EVP_PKEY instance is required from multiple threads without locking. In OpenSSL 3.0 when the evp_pkey_downgrade function is called, which downgrades an EVP_PKEY instance to a legacy version, it will clear all the fields of EVP_PKEY struct except the lock (nodejs#13374). But this also means that keymgmt and keydata will also be cleared, which other parts of the code base depends on, and those calls will either fail to export the key (returning null) or crash due to a segment fault. This same code works with OpenSSL 1.1.1 without locking and I think this is because there is no downgrade being done in OpenSSL 1.1.1. But even so, as far as I can tell there are no guarantees that these object are thread safe in 1.1.1 either and should be protected with a lock. Refs: openssl/openssl#13374 openssl/openssl#2165) https://www.openssl.org/blog/blog/2017/02/21/threads

This commit introduces a Mutex field on the ManagedEVPPKey class intended to be used when multiple threads require access to an OpenSSL EVP_PKEY object. The motivation for this is that OpenSSL objects like EVP_PKEY are not thread safe. In versions prior to OpenSSL 3.0 this was not noticed and did not cause any issues, like incorrect logic or crashes, but with OpenSSL 3.0 this does cause problems if access to an EVP_PKEY instance is required from multiple threads without locking. In OpenSSL 3.0 where the evp_pkey_downgrade function is called, which downgrades an EVP_PKEY instance to a legacy version, it will clear all the fields of EVP_PKEY struct except the lock (#13374). But this will also means that keymgmt and keydata will also be cleared which other parts of the code base depends upon, and those calls will either fail to export the key (returning null) or crash due to a segment fault. This same code works with OpenSSL 1.1.1 and I'm guessing this is because there is no downgrade in OpenSSL 1.1.1 (there is only the now legacy struct) and the above situation never happens. Refs: openssl/openssl#13374 openssl/openssl#2165) https://www.openssl.org/blog/blog/2017/02/21/threads

This commit introduces a mutex field on the ManagedEVPPKey class intended to be used when multiple threads require access to an OpenSSL EVP_PKEY object. The motivation for this came from the work being done to upgrade Node.js to OpenSSL 3.0. OpenSSL objects, like EVP_PKEY, are not thread safe (see refs for details). In versions prior to OpenSSL 3.0 this was not noticeable and did not cause any issues (like incorrect logic or crashes), but with OpenSSL 3.0 this does cause issues if access to an EVP_PKEY instance is required from multiple threads without locking. In OpenSSL 3.0 when the evp_pkey_downgrade function is called, which downgrades an EVP_PKEY instance to a legacy version, it will clear all the fields of EVP_PKEY struct except the lock (nodejs#13374). But this also means that keymgmt and keydata will also be cleared, which other parts of the code base depends on, and those calls will either fail to export the key (returning null) or crash due to a segment fault. This same code works with OpenSSL 1.1.1 without locking and I think this is because there is no downgrade being done in OpenSSL 1.1.1. But even so, as far as I can tell there are no guarantees that these object are thread safe in 1.1.1 either and should be protected with a lock. Refs: openssl/openssl#13374 openssl/openssl#2165) https://www.openssl.org/blog/blog/2017/02/21/threads

This commit introduces a mutex field on the ManagedEVPPKey class intended to be used when multiple threads require access to an OpenSSL EVP_PKEY object. The motivation for this came from the work being done to upgrade Node.js to OpenSSL 3.0. OpenSSL objects, like EVP_PKEY, are not thread safe (see refs for details). In versions prior to OpenSSL 3.0 this was not noticeable and did not cause any issues (like incorrect logic or crashes), but with OpenSSL 3.0 this does cause issues if access to an EVP_PKEY instance is required from multiple threads without locking. In OpenSSL 3.0 when the evp_pkey_downgrade function is called, which downgrades an EVP_PKEY instance to a legacy version, it will clear all the fields of EVP_PKEY struct except the lock (#13374). But this also means that keymgmt and keydata will also be cleared, which other parts of the code base depends on, and those calls will either fail to export the key (returning null) or crash due to a segment fault. This same code works with OpenSSL 1.1.1 without locking and I think this is because there is no downgrade being done in OpenSSL 1.1.1. But even so, as far as I can tell there are no guarantees that these object are thread safe in 1.1.1 either and should be protected with a lock. PR-URL: #36825 Refs: openssl/openssl#13374 Refs: openssl/openssl#13374 Refs: openssl/openssl#2165) Refs: https://www.openssl.org/blog/blog/2017/02/21/threads Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Jun 1, 2017

jasnell mentioned this pull request Jun 1, 2017

zlib: fix inheritance of DeflateRaw without classes #13370

Closed

3 tasks

addaleax approved these changes Jun 1, 2017

View reviewed changes

refack approved these changes Jun 1, 2017

View reviewed changes

jasnell force-pushed the fix-zlib-take2 branch from 1054a6a to f63b2cb Compare June 1, 2017 19:00

jasnell added dont-land-on-v4.x labels Jun 1, 2017

mscdex reviewed Jun 1, 2017

View reviewed changes

watilde reviewed Jun 1, 2017

View reviewed changes

zlib: revert back to Functions

72571d2

Using ES6 Classes broke userland code. Revert back to functions. Fixes: nodejs#13358 Refs: nodejs#13370

jasnell force-pushed the fix-zlib-take2 branch from f63b2cb to 72571d2 Compare June 1, 2017 20:17

watilde approved these changes Jun 1, 2017

View reviewed changes

This was referenced Jun 1, 2017

v8.0.0 — zlib.DeflateRaw only extensible via class keyword #13358

Closed

stream,zlib: performance improvements #13322

Closed

mcollina approved these changes Jun 2, 2017

View reviewed changes

watilde mentioned this pull request Jun 2, 2017

Throws error in node v8.0.0 archiverjs/node-archiver#236

Closed

mbrevda mentioned this pull request Jun 5, 2017

error with firebase deploy functions firebase/firebase-tools#356

Closed

jasnell closed this Jun 5, 2017

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

mymattcarroll mentioned this pull request Jun 6, 2017

Throws error in node v8.0.0 oneblink/server-cli#224

Closed

franciscocpg mentioned this pull request Jun 8, 2017

TypeError: Cannot read property 'pipesCount' of undefined archiverjs/node-compress-commons#25

Closed

danbev mentioned this pull request Jan 7, 2021

src: add mutex to ManagedEVPPKey class #36825

Closed

aliscco mentioned this pull request Apr 27, 2023

[Snyk] Fix for 30 vulnerabilities aliscco/alisco-node#279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: revert back to Functions #13374

zlib: revert back to Functions #13374

jasnell commented Jun 1, 2017

refack left a comment

jasnell commented Jun 1, 2017

jasnell commented Jun 1, 2017

jasnell commented Jun 1, 2017

mscdex Jun 1, 2017 •

edited

Loading

watilde Jun 1, 2017

jasnell commented Jun 1, 2017

mcollina left a comment

jasnell commented Jun 5, 2017

zlib: revert back to Functions #13374

zlib: revert back to Functions #13374

Conversation

jasnell commented Jun 1, 2017

Checklist

Affected core subsystem(s)

refack left a comment

Choose a reason for hiding this comment

jasnell commented Jun 1, 2017

jasnell commented Jun 1, 2017

jasnell commented Jun 1, 2017

mscdex Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

watilde Jun 1, 2017

Choose a reason for hiding this comment

jasnell commented Jun 1, 2017

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Jun 5, 2017

mscdex Jun 1, 2017 •

edited

Loading