tls: use the most recently added matching SecureContext in default SNICallback. #34638
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
addaleax
approved these changes
Aug 5, 2020
|
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reverse The reverse() method reverses an array in place. The first array element becomes the last, and the last array element becomes the first. Array.prototype.reverse() This method will change the original array, which produces unnecessary side effects and will cause a great performance loss. |
|
We should not use the Array.prototype.reverse() method. For the same servername, when the server.addContext and SNICallback method is used more than 2 times, an error will occur in half of the cases. |
|
Since we are solving the same problem, could you help me write a test for this pull request? |
|
Thank you @masx200 , good catch. Unfortunately, your PR contains numerous linting errors that you should be aware of, if you actually ran Regarding tests - below guide for writing tests should be helpful: |
bnoordhuis
approved these changes
Aug 6, 2020
|
It seems that there are two competing PRs for this? Which one should land? |
|
Well I reported the issue and stated I have a WIP fix. And the other one still has multiple unresolved ssues. |
|
Seems to have gotten a little stuck. What do we need to move further? |
|
@nodejs/crypto |
Refs: web-platform-tests/wpt#25987 PR-URL: nodejs#35636 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
It is already installed in the GitHub runners. PR-URL: nodejs#35638 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
The other way is deprecated. PR-URL: nodejs#35638 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
This adds support for DNS Certification Authority Authorization (RFC 8659) to Node.js. PR-URL: nodejs#35466 Fixes: nodejs#19239 Refs: nodejs#14713 Reviewed-By: Anna Henningsen <[email protected]>
0 is deprecated. Change to 1, which is experimental. PR-URL: nodejs#35672 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#35642 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Since the section refers to EventEmitter, instances in the example should be created of the same class EventEmitter. PR-URL: nodejs#33513 Reviewed-By: James M Snell <[email protected]>
Currently we are returning the stringified error code while in other process methods we are throwin a UVException and only exclusion is in the CPUUsage. Converted it to follow the convention. PR-URL: nodejs#34762 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs#35086 Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#35522 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Use `common.mustCall` and `util.debuglog`. Remove unnecessary functions PR-URL: nodejs#32805 Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#35690 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#35653 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
PR-URL: nodejs#35657 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#35593 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
This is in preparation for updating remark-preset-lint-node to 1.17.1. PR-URL: nodejs#35668 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Original commit message:
Fix alloc/dealloc size mismatch for v8::BackingStore
On newer compilers the {operator delete} with explicit {size_t}
argument would be instantiated for {v8::BackingStore} and used
in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
argument is wrong though, since the pointer actually points
to a {v8::internal::BackingStore} object.
The solution is to explicitly provide a {operator delete}, preventing
an implicitly generated {size_t} operator.
Bug:v8:11081
Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Camillo Bruni <[email protected]>
Cr-Commit-Position: refs/heads/master@{#70916}
PR-URL: nodejs#35939
Fixes: nodejs#35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
A test was added for this in 6f34498 but because it was a test in the `internet` directory, it was not run on CI and it was not noticed that the test was failing. This fixes the error that was causing the test to fail. PR-URL: nodejs#35979 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#36017 Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#27413 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#32655 Fixes: nodejs#32559 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Myles Borins <[email protected]>
PR-URL: nodejs#34543 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
|
@mkrawczuk can you fix the linter issue please? |
aduh95
added
stalled
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
Refs: nodejs#34944 (comment) Co-authored-by: Anna Henningsen <[email protected]> PR-URL: nodejs#35000 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs#35197 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
In stream write encoding can be null. Fixes: nodejs#33715 PR-URL: nodejs#35372 Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#35633 Fixes: nodejs#35600 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#35999 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>
`EventTarget` is exposed on the global scope, there is no need to use `--expose-internals` flag in the tests. Refs: nodejs#35496 PR-URL: nodejs#36002 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs#35993 (comment) PR-URL: nodejs#35995 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#36024 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Make the linter recommend replacing `globalThis.Map` by `primordials.SafeMap`, and similar for `Set`, `WeakSet`, and `WeakMap`. PR-URL: nodejs#36026 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
PR-URL: nodejs#35679 Fixes: nodejs#35629 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#36012 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Don't write to a stream which already has a full buffer. Fixes: nodejs#35341 PR-URL: nodejs#35348 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Search the default installation path for NASM installed by a user without administrator privileges when not found on the Path or in the default system-wide installation path. PR-URL: nodejs#36014 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
4 tasks
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.
Fixes: #34110
make -j4 test(UNIX), orvcbuild test(Windows) passes