[v18.x] Backport 48969 to v18.x staging by mhdawson · Pull Request #49016 · nodejs/node

and others added 9 commits

July 23, 2023 17:23
The test's assumptions about RSS are no longer valid, at least with
Fedora 38.

Closes: nodejs#48490
PR-URL: nodejs#48811
Fixes: nodejs#48490
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#45777
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.

PR-URL: nodejs#48258
Backport-PR-URL: nodejs#48275
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Previously persistence was completed disabled if you removed this
header, which is not correct for modern HTTP, where the header is
optional and all connections should persist by default regardless.

PR-URL: nodejs#46331
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
PR-URL: nodejs#46587
Backport-PR-URL:
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
And switch from `google.com` to `nodejs.org`.

PR-URL: nodejs#47029
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Fixs two issues in `TLSWrap`, one of them is reported in
nodejs#30896.

1. `TLSWrap` has exactly one `StreamListener`, however,
that `StreamListener` can be replaced. We have not been
rigorous enough here: if an active write has not been
finished before the transition, the finish callback of it
will be wrongly fired the successor `StreamListener`.

2. A `TLSWrap` does not allow more than one active write,
as checked in the assertion about current_write in
`TLSWrap::DoWrite()`.

However, when users make use of an existing `tls.TLSSocket`
to establish double TLS, by
either
  tls.connect({socket: tlssock})
or
  tlsServer.emit('connection', tlssock)
we have both of the user provided `tls.TLSSocket`, tlssock and
a brand new created `TLSWrap` writing to the `TLSWrap` bound to
tlssock, which easily violates the constranint because two writers
have no idea of each other.

The design of the fix is:
when a `TLSWrap` is created on top of a user provided socket,
do not send any data to the socket until all existing writes
of the socket are done and ensure registered callbacks of
those writes can be fired.

PR-URL: nodejs#48969
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
- APLN option is not supported on 18.x so remove

Signed-off-by: Michael Dawson <mdawson@devrus.com>

@nodejs-github-bot nodejs-github-bot added meta

Issues and PRs related to the general management of the project.

tools

Issues and PRs related to the tools directory.

labels

Aug 4, 2023

@RafaelGSS RafaelGSS changed the base branch from main to v18.x-staging

August 4, 2023 15:08

@RafaelGSS RafaelGSS changed the title Backport 48969 to v18.x staging [v18.x] Backport 48969 to v18.x staging

Aug 4, 2023

@RafaelGSS RafaelGSS added the v18.x

Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

label

Aug 4, 2023

mhdawson

@mhdawson

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
PR-URL: #45777
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
PR-URL: #47860
Backport-PR-URL: #49016
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.

PR-URL: #48258
Backport-PR-URL: #49016
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
Previously persistence was completed disabled if you removed this
header, which is not correct for modern HTTP, where the header is
optional and all connections should persist by default regardless.

PR-URL: #46331
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
PR-URL: #46587
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
And switch from `google.com` to `nodejs.org`.

PR-URL: #47029
Backport-PR-URL: #49016
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

ruyadorno pushed a commit that referenced this pull request

Aug 14, 2023
Fixs two issues in `TLSWrap`, one of them is reported in
#30896.

1. `TLSWrap` has exactly one `StreamListener`, however,
that `StreamListener` can be replaced. We have not been
rigorous enough here: if an active write has not been
finished before the transition, the finish callback of it
will be wrongly fired the successor `StreamListener`.

2. A `TLSWrap` does not allow more than one active write,
as checked in the assertion about current_write in
`TLSWrap::DoWrite()`.

However, when users make use of an existing `tls.TLSSocket`
to establish double TLS, by
either
  tls.connect({socket: tlssock})
or
  tlsServer.emit('connection', tlssock)
we have both of the user provided `tls.TLSSocket`, tlssock and
a brand new created `TLSWrap` writing to the `TLSWrap` bound to
tlssock, which easily violates the constranint because two writers
have no idea of each other.

The design of the fix is:
when a `TLSWrap` is created on top of a user provided socket,
do not send any data to the socket until all existing writes
of the socket are done and ensure registered callbacks of
those writes can be fired.

PR-URL: #48969
Backport-PR-URL: #49016
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>

This was referenced

Aug 17, 2023