Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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>
- Loading branch information
Showing
7 changed files
with
297 additions
and
24 deletions.
There are no files selected for viewing
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 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 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 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| 'use strict'; | ||
| const common = require('../common'); | ||
| const assert = require('assert'); | ||
| if (!common.hasCrypto) common.skip('missing crypto'); | ||
| const fixtures = require('../common/fixtures'); | ||
| const tls = require('tls'); | ||
|
|
||
| // In reality, this can be a HTTP CONNECT message, signaling the incoming | ||
| // data is TLS encrypted | ||
| const HEAD = 'XXXX'; | ||
|
|
||
| const subserver = tls.createServer({ | ||
| key: fixtures.readKey('agent1-key.pem'), | ||
| cert: fixtures.readKey('agent1-cert.pem'), | ||
| }) | ||
| .on('secureConnection', common.mustCall(() => { | ||
| process.exit(0); | ||
| })); | ||
|
|
||
| const server = tls.createServer({ | ||
| key: fixtures.readKey('agent1-key.pem'), | ||
| cert: fixtures.readKey('agent1-cert.pem'), | ||
| }) | ||
| .listen(client) | ||
| .on('secureConnection', (serverTlsSock) => { | ||
| serverTlsSock.on('data', (chunk) => { | ||
| assert.strictEqual(chunk.toString(), HEAD); | ||
| subserver.emit('connection', serverTlsSock); | ||
| }); | ||
| }); | ||
|
|
||
| function client() { | ||
| const down = tls.connect({ | ||
| host: '127.0.0.1', | ||
| port: server.address().port, | ||
| rejectUnauthorized: false | ||
| }).on('secureConnect', () => { | ||
| down.write(HEAD, common.mustSucceed()); | ||
|
|
||
| // Sending tls data on a client TLSSocket with an active write led to a crash: | ||
| // | ||
| // node[16862]: ../src/crypto/crypto_tls.cc:963:virtual int node::crypto::TLSWrap::DoWrite(node::WriteWrap*, | ||
| // uv_buf_t*, size_t, uv_stream_t*): Assertion `!current_write_' failed. | ||
| // 1: 0xb090e0 node::Abort() [node] | ||
| // 2: 0xb0915e [node] | ||
| // 3: 0xca8413 node::crypto::TLSWrap::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [node] | ||
| // 4: 0xcaa549 node::StreamBase::Write(uv_buf_t*, unsigned long, uv_stream_s*, v8::Local<v8::Object>) [node] | ||
| // 5: 0xca88d7 node::crypto::TLSWrap::EncOut() [node] | ||
| // 6: 0xd3df3e [node] | ||
| // 7: 0xd3f35f v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node] | ||
| // 8: 0x15d9ef9 [node] | ||
| // Aborted | ||
| tls.connect({ | ||
| socket: down, | ||
| rejectUnauthorized: false | ||
| }); | ||
| }); | ||
| } |
Oops, something went wrong.