Closed
Conversation
3 tasks
addaleax
approved these changes
Jun 1, 2017
refack
approved these changes
Jun 1, 2017
Contributor
refack
left a comment
There was a problem hiding this comment.
Maybe take the test from #13370 https://github.com/nodejs/node/pull/13370/files#diff-01422010a61e75fcb72efe054d70972b
Member
Author
|
heh... was just pushing that up @refack :-) |
Member
Author
Member
Author
|
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
reviewed
Jun 1, 2017
lib/zlib.js
Outdated
Contributor
There was a problem hiding this comment.
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
reviewed
Jun 1, 2017
lib/zlib.js
Outdated
Member
There was a problem hiding this comment.
I think we had these two descriptors at here previously.
configurable: true,
enumerable: trueUsing ES6 Classes broke userland code. Revert back to functions. Fixes: nodejs#13358 Refs: nodejs#13370
Member
Author
|
Updated to address nits |
watilde
approved these changes
Jun 1, 2017
This was referenced Jun 1, 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>
Member
Author
|
Landed in 7024c5a |
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>
Closed
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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), orvcbuild test(Windows) passesAffected core subsystem(s)
zlib