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

zlib: revert back to Functions #13374

Closed
wants to merge 1 commit into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 1, 2017

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

/cc @mcollina @addaleax @watilde

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

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

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell
Copy link
Member Author

jasnell commented Jun 1, 2017

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

@jasnell
Copy link
Member Author

jasnell commented Jun 1, 2017

@jasnell
Copy link
Member Author

jasnell commented Jun 1, 2017

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.

lib/zlib.js Outdated
opts = opts || {};
super(opts);
function Zlib(opts, mode) {
if (!(this instanceof Zlib))
Copy link
Contributor

@mscdex mscdex Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/zlib.js Outdated

get _closed() {
Object.defineProperty(Zlib.prototype, '_closed', {
get() {
return !this._handle;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

jasnell commented Jun 1, 2017

Updated to address nits

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasnell added a commit that referenced this pull request Jun 5, 2017
Using ES6 Classes broke userland code. Revert back to functions.

PR-URL: #13374
Fixes: #13358
Ref: #13370
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Jun 5, 2017

Landed in 7024c5a

@jasnell jasnell closed this Jun 5, 2017
jasnell added a commit that referenced this pull request Jun 5, 2017
Using ES6 Classes broke userland code. Revert back to functions.

PR-URL: #13374
Fixes: #13358
Ref: #13370
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
jasnell added a commit to jasnell/node that referenced this pull request Jun 7, 2017
* **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).
rvagg pushed a commit that referenced this pull request Jun 8, 2017
* **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).
rvagg pushed a commit that referenced this pull request Jun 8, 2017
* **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).
hhellyer pushed a commit to nodejs/llnode that referenced this pull request Jun 29, 2017
* 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 <fedor@indutny.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danbev added a commit to danbev/node that referenced this pull request Jan 7, 2021
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
danbev added a commit that referenced this pull request Jan 11, 2021
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
danbev added a commit that referenced this pull request Jan 12, 2021
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
danbev added a commit to danbev/node that referenced this pull request Jan 21, 2021
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
danbev added a commit to danbev/node that referenced this pull request Feb 9, 2021
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
danbev added a commit that referenced this pull request Feb 10, 2021
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 <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
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 <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v8.0.0 — zlib.DeflateRaw only extensible via class keyword
7 participants