Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: fix bugs of double TLS #48969

Closed
wants to merge 6 commits into from

Conversation

ywave620
Copy link
Contributor

A replacement of #48796
Fix a 3y long bug #30896

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.
Remove unnecessary saving of `this` for arrow function.
Introduce enum UnderlyingStreamWriteStatus.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 30, 2023
@ywave620
Copy link
Contributor Author

ywave620 commented Aug 2, 2023

@mhdawson PTAL

@mhdawson
Copy link
Member

mhdawson commented Aug 2, 2023

@ywave620 I had landed your original version in this PR. What I'd hope for was a PR that just addressed the remaining suggestion from @jasnell.

Maybe this just needs to be rebased on the latest version of main to become that?

@ywave620
Copy link
Contributor Author

ywave620 commented Aug 3, 2023

I had landed your original version in this PR.

I checked the main branch and didn't spot it

just addressed the remaining suggestion from @jasnell.

Included in the last commit of this PR

@mhdawson

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2023

@ywave620 could you rebase on top of the current main instead of adding more commits to the PR. Ideally we'd have a PR with a single commit that addresses @jasnell's comment.

My mistake

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2023

@ywave620 sorry for the confusion. Seems I missed pushing the commit for the last PR to main somehow. So this is what we need:)

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2023

Windows failure was a known flaky test, resumed build.

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2023

osx failures look unrelated to this PR.

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2023

This time failure looked like a machine issue, resuming again.

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2023

And again 2 tests with issues indicating they are flaky on windows, resumed again.

pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
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>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
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>
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>
@ruyadorno ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. and removed needs-ci PRs that need a full CI run. backported-to-v18.x PRs backported to the v18.x-staging branch. labels Aug 14, 2023
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
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>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
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>
@mhdawson
Copy link
Member

Backport to 18.x PR (second try) - #49183

RafaelGSS pushed a commit that referenced this pull request Aug 15, 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
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
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>
@mhdawson mhdawson added the backport-open-v18.x Indicate that the PR has an open backport. label Aug 15, 2023
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
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>
rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 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
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
ruyadorno pushed a commit that referenced this pull request Aug 17, 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: #49183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@ruyadorno ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v18.x Indicate that the PR has an open backport. labels Aug 17, 2023
@ruyadorno ruyadorno mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants