zlib: revert back to Functions#13374
Closed
+329 −293
Conversation
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
3 tasks
addaleax approved these changes Jun 1, 2017
refack approved these changes Jun 1, 2017
Contributor
refack left a comment
refack left a comment There was a problem hiding this comment.
Maybe take the test from #13370https://github.com/nodejs/node/pull/13370/files#diff-01422010a61e75fcb72efe054d70972b
MemberAuthor
jasnell commented Jun 1, 2017
heh... was just pushing that up @refack :-) |
MemberAuthor
jasnell commented Jun 1, 2017
MemberAuthor
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. |
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: true
Using ES6 Classes broke userland code. Revert back to functions. Fixes: nodejs#13358 Refs: nodejs#13370
MemberAuthor
jasnell commented Jun 1, 2017
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: #13374Fixes: #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]>
MemberAuthor
jasnell commented Jun 5, 2017
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: #13374Fixes: #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]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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#13374openssl/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#13374openssl/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#13374openssl/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#13374openssl/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#13374openssl/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 <[email protected]>
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 <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
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