This repository has been archived by the owner on Mar 4, 2020. It is now read-only.
fix: remove array buffer allocator patch (electron-4.x) #110
Conversation
This file contains 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
This reverts commit e66aa77.
EC_KEY_key2buf returns an OPENSSL_malloc'd pointer so it shouldn't be passed into Buffer::New, which expect a libc malloc'd pointer. Instead, factor out the ECDH::GetPublicKey code which uses EC_POINT_point2oct. This preserves the existing behavior where encoding failures are silently ignored, but it is probably safe to CHECK fail them instead. PR-URL: nodejs/node#25717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use MaybeLocal::ToLocal and don't crash X509ToObject on error. PR-URL: nodejs/node#25717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Always use the right allocator for memory that is turned into an `ArrayBuffer` at a later point. This enables embedders to use their own `ArrayBuffer::Allocator`s, and is inspired by Electron’s f61bae3440e. It should render their downstream patch unnecessary. Refs: f61bae3 PR-URL: nodejs/node#26207 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This enables us to identify whether we are using an allocator that we know more about than what the generic `ArrayBuffer::Allocator` API provides, in particular whether it is `malloc()`-compatible. PR-URL: nodejs/node#26207 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s in the current `Isolate` is not a Node.js `ArrayBufferAllocator`, we cannot know that it is `malloc()`-based, an in particular it might not be compatible with the `ArrayBuffer::Allocator` on the receiving end of the connection. PR-URL: nodejs/node#26207 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Add a subclass of `ArrayBufferAllocator` that performs additional debug checking, which in particular verifies that: - All `ArrayBuffer` backing stores have been allocated with this allocator, or have been explicitly marked as coming from a compatible source. - All memory allocated by the allocator has been freed once it is destroyed. PR-URL: nodejs/node#26207 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Add a RAII utility for managing blocks of memory that have been allocated with the `ArrayBuffer::Allocator` for a given `Isolate`. PR-URL: nodejs/node#26207 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Throw an exception instead of crashing when attempting to create `Buffer` objects from a Context that is not associated with a Node.js `Environment`. Possible alternatives for the future might be just returning a plain `Uint8Array`, or working on providing `Buffer` for all `Context`s. PR-URL: nodejs/node#23938 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
This call was introduced in 827ee49 to avoid a crash in a later `Neuter()` call that has later been removed in ebbbc5a, rendering the original call unnecessary. Refs: nodejs/node#3624 Refs: nodejs/node#5204 PR-URL: nodejs/node#25479 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc. Additionally, though OpenSSL doesn't type-check due to it being a macro, the function is documented to take an unsigned char pointer: https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html (By default, OPENSSL_malloc is the same as libc malloc, but it is possible to customize this.) PR-URL: nodejs/node#25706 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
|
/cc @addaleax would be great to get your review whenever possible. Thanks! |
deepak1556
changed the title
addaleax
approved these changes
Jul 10, 2019
There was a problem hiding this comment.
I think you also want nodejs/node@b84b4d8, it fixes a regression from nodejs/node#26207, but that’s about it :)
Track received data correctly. Specifically, for the buffer that is used for receiving data, we previously would try to increment the current memory usage by its length, and later decrement it by that, but in the meantime the buffer had been turned over to V8 and its length reset to zero. This gave the impression that more and more memory was consumed by the HTTP/2 session when it was in fact not. Fixes: nodejs/node#27416 Refs: nodejs/node#26207 PR-URL: nodejs/node#27914 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
deepak1556
changed the title
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR removes our array buffer allocator patch for 10.11.0 branch and replace it with official version from nodejs/node#26207 . The main reason being the patch isn't audited with every node version upgrade and is source of potential allocation errors https://github.com/electron/electron/search?q=set+a+breakpoint+in+malloc_error_break+to+debug&type=Issues .
In VSCode based on electron 4.x we have been seeing these crashes microsoft/vscode#76197.
We don't have a reliable repro for the crashes because the symbols are borked, hoping to get some data with electron/electron#19158 in future releases. So at the moment I decided to choose the route of removing our patch for the official version as a possible solution.
I don't have hopes for this solution getting into electron 4.x with it being a stable release, but would still like to get a review. Thanks!