[v10.x] src: more automatic memory management in node_crypto.cc #20609
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
Also, fix some other nits in passing (formatting, punctuation, typos, redundancy, obsoleteness). PR-URL: nodejs#20438 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Although I agree with the guideline people should generally not squash commits in a pull request until the end (in other words, until it's time to land the PR), it is clear from comments and actions in the issue tracker that many do not share that view. This is fine by me, but I do think that we should our documentation should reflect our practices rather than being an aspirational statement. If we *do* wish to preserve this recommendation, it probably belongs in another document anyway as this is not a recommendation for Collaborators only but for anyone opening a pull request. PR-URL: nodejs#20413 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>
Tell the contributor to generally not squash commits during the pull request review process. PR-URL: nodejs#20413 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>
A recent change made these benchmarks fail by always finishing with 0 iterations. Restore a counter variable. PR-URL: nodejs#20461 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]>
In http.ClientRequest's doc, add maxHeadersCount as a public property. And in the description of server's one, change a hyphen to a comma. PR-URL: nodejs#20361 Refs: nodejs#20359 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Calculated durations are timedelta objects but the FormatTime function is expecting a number in seconds. PR-URL: nodejs#20368 Fixes: nodejs#20341 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: nodejs@368517c PR-URL: nodejs#20379 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#20397 Refs: nodejs#8913 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Refs: nodejs#8913 PR-URL: nodejs#20399 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#20343 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Use the potentially more efficient fs.copyFileSync() instead of reading the whole file in and writing the whole file out in JavaScript. PR-URL: nodejs#20340 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#20469 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Instead of always checking whether we've already warned about a possible EventEmitter memory leak, first run the rest of the code as accessing random properties on an Array is expensive. In addition, remove an unnecessary truthy check. PR-URL: nodejs#20452 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Do less variable allocations and reassignments inside spliceOne since it's relied on by some performance sensitive code. PR-URL: nodejs#20453 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: nodejs#20400 Fixes: nodejs#20385 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]>
The documentation used `file` for the first argument to `appendFile()` functions. However, the code and (more importantly) thrown errors referred to it as `path`. The latter is especially important because context is not provided. So you're looking for a function that takes `path` but that string doesn't appear in your code *or* in the documentation. It's not until the end user looks at the source code of Node.js that they can figure out what's going on. This is why it is important that the names of variables in the documentation match that in the code. If we want to change this to `file`, then that's OK, but we need to do it in the source code and error messages too, not just in the docs. Changing the docs is the smallest change to synchronize everything so that's what this change does. PR-URL: nodejs#20489 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: nodejs#20408 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
PR-URL: nodejs#20408 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
In pull-requests.md: * Refer to the Collaborator Guide as Collaborator Guide and not Collaborator's Guide. That is how the doc describes itself and we should be consistent. PR-URL: nodejs#20473 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Partial doc cleanup as per nodejs#20421 PR-URL: nodejs#20430 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
This was originally introduced in 3446ff4, in order to fix a hard crash. However, since the libuv 1.18.0 update, that hard crash is gone, and since f2b9805 we do not throw an error in JS land anymore either, rendering the flag unnecessary. Also, the original test that checked this condition was added to `test/parallel/`. Since that typically runs without a TTY stdin, a duplicate test is being added to the pseudo-tty test suite in this commit. Refs: nodejs@3446ff4 Refs: nodejs@f2b9805 Refs: libuv/libuv@0e28141 PR-URL: nodejs#20388 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#20455 Fixes: nodejs#18897 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
- Instead of storing a pointer whose type refers to the specific subclass of `BaseObject`, just store a `BaseObject*` directly. This means in particular that one can cast to classes along the way of the inheritance chain without issues, and that `BaseObject*` no longer needs to be the first superclass in the case of multiple inheritance. In particular, this renders hack-y solutions to this problem (like ddc19be) obsolete and addresses a `TODO` comment of mine. - Move wrapping/unwrapping methods to the `BaseObject` class. We use these almost exclusively for `BaseObject`s, and I hope that this gives a better idea of how (and for what) these are used in our code. - Perform initialization/deinitialization of the internal field in the `BaseObject*` constructor/destructor. This makes the code a bit more obviously correct, avoids explicit calls for this in subclass constructors, and in particular allows us to avoid crash situations when we previously called `ClearWrap()` during GC. This also means that we enforce that the object passed to the `BaseObject` constructor needs to have an internal field. This is the only reason for the test change. - Change the signature of `MakeWeak()` to not require a pointer argument. Previously, this would always have been the same as `this`, and no other value made sense. Also, the parameter was something that I personally found somewhat confusing when becoming familiar with Node’s code. - Add a `TODO` comment that motivates switching to real inheritance for the JS types we expose from the native side. This patch brings us a lot closer to being able to do that. - Some less significant drive-by cleanup. Since we *effectively* already store the `BaseObject*` pointer anyway since ddc19be, I do not think that this is going to have any impact on diagnostic tooling. Fixes: nodejs#18897 PR-URL: nodejs#20455 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: nodejs#20460 Fixes: nodejs#17508 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: nodejs#20373 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <[email protected]> PR-URL: nodejs#20370 Fixes: nodejs#19655 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#20480 Refs: v8/v8@6.6.346.24...6.6.346.27 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Several minor fixes to the entries for `mkdtemp()`. The most significant is that a mistaken use of `fs.mkdtemp()` is corrected to `fsPromises.mkdtemp()`. PR-URL: nodejs#20512 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
Document that REPL uses the `domain` module to handle uncaught exceptions, and the side effects caused by it. PR-URL: nodejs#20382 Fixes: nodejs#19998 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Added test for fs/promises filehandle stat. PR-URL: nodejs#20492 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>
* "un-deprecation" ಠ_ಠ -> "revoking deprecations" * "From time-to-time" -> "Occastionally" * "semver-major" and "semver-minor" are jargon that readers who don't follow our issue tracker will not know. Remove the sentence as it doesn't really impact end users. The deprecation is revoked when it is revoked. Rules around the releases where deprecations can be revoked may be added to the COLLABORATOR_GUIDE in the extensive section about deprecations there. If so, great, but let's still remove it here as having the information scattered in two places makes it likely that one will be edited to contradict the other and then it won't be clear which one is correct. * Remove unneeded italics. The italicized sentence is not hugely critical information that we desperately want users to know. Most users won't care. They will only care about the deprecation message that they are looking up at that moment. PR-URL: nodejs#20519 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
"periodically" implies regular time intervals between emitted events, but as first example, "peer connects", implies, the time intervals may be irregular or unpredictable. PR-URL: nodejs#20581 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
Some of the mentioned files seem to be moved or renamed. PR-URL: nodejs#20564 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#20504 Refs: nodejs/TSC#389 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Add the trace category for file system synchronous methods to documentation so the users can enable it when they want to look into file system sync method trace data. PR-URL: nodejs#20526 Refs: nodejs#19649 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
This makes `console.table()` inspect objects with color if available like `console.log()`. PR-URL: nodejs#20510 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: James M Snell <[email protected]>
tunniclm indicated that it would be a good idea to move them to Collaborator Emeritus for now. This commit makes that change in the README.md file. PR-URL: nodejs#20533 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: nodejs#20407 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jamie Davis <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This was a clear error. chown should do chown, not chmod. PR-URL: nodejs#20407 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jamie Davis <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
this === eventEmitter or this === instance of EventEmitter, but it's this is not EventEmitter. PR-URL: nodejs#20537 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
PR-URL: nodejs#20586 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This commit replaces two ad hoc symlink permission tests with common.canCreateSymLink(). PR-URL: nodejs#20540 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Add a custom eslint rule to check for `common.skipIfEslintMissing()` to allow tests to run from source tarballs that do not include eslint. Fix up rule tests that were failing the new check. Refs: nodejs#20336 PR-URL: nodejs#20372 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#20599 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Prefer custom smart pointers fitted to the OpenSSL data structures over more manual memory management and lots of `goto`s. PR-URL: nodejs#20238 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot
added
c++
MylesBorins
force-pushed
the
v10.x-staging
branch
2 times, most recently
from
a24b15d to
925939a
Compare
MylesBorins
pushed a commit
that referenced
this pull request
May 8, 2018
Prefer custom smart pointers fitted to the OpenSSL data structures over more manual memory management and lots of `goto`s. Backport-PR-URL: #20609 PR-URL: #20238 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
landed in 925939a |
|
Closing in favor of #20706 |
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.
Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of
gotos.Had only 3 minor merge conflicts (
foo→foo.get()in API calls).Original PR-URL: #20238
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes