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

TLSSocket.bufferSize is always +1 before and after write() #15005

Closed
micooz opened this issue Aug 24, 2017 · 0 comments
Closed

TLSSocket.bufferSize is always +1 before and after write() #15005

micooz opened this issue Aug 24, 2017 · 0 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@micooz
Copy link

micooz commented Aug 24, 2017

  • Version: v8.4.0
  • Platform: Linux 4.4.0-92-generic x86_64 GNU/Linux
  • Subsystem: net,tls

reproduce:

const tls = require('tls');
const fs = require('fs');

const key = fs.readFileSync('key.pem');
const cert = fs.readFileSync('cert.pem');

const server = tls.createServer({key, cert}, (socket) => {
  socket.pipe(socket);
  socket.on('end', () => {
    server.close();
  });
});

server.listen(0, () => {
  const client = tls.connect(server.address().port, {rejectUnauthorized: false}, () => {
    console.log(client.bufferSize); // 1

    client.write(Buffer.alloc(10));
    console.log(client.bufferSize); // 11

    client.end();
  });

  client.on('finish', () => {
    console.log(client.bufferSize); // 1
  });
});

bufferSize is obviously wrong in this case, this affects users who rely on this property to make judgments, for example:

if (socket.bufferSize <= 0) {
  // oops! never reach here...
}

why?

node/lib/_tls_wrap.js

Lines 427 to 437 in 3cf27f1

TLSSocket.prototype._init = function(socket, wrap) {
var self = this;
var options = this._tlsOptions;
var ssl = this._handle;
// lib/net.js expect this value to be non-zero if write hasn't been flushed
// immediately
// TODO(indutny): revise this solution, it might be 1 before handshake and
// represent real writeQueueSize during regular writes.
ssl.writeQueueSize = 1;

It seems a legacy code here. After I searched the entire lib, writeQueueSize is only use for calculating bufferSize and decide if should wait until write flushed.

node/lib/net.js

Lines 465 to 471 in 3cf27f1

Object.defineProperty(Socket.prototype, 'bufferSize', {
get: function() {
if (this._handle) {
return this._handle.writeQueueSize + this._writableState.length;
}
}
});

node/lib/net.js

Lines 768 to 773 in 3cf27f1

// If it was entirely flushed, we can write some more right now.
// However, if more is left in the queue, then wait until that clears.
if (req.async && this._handle.writeQueueSize !== 0)
req.cb = cb;
else
cb();

Simply remove writeQueueSize works fine for me, and I also opened a PR to fix the problem, all local tests passed but I still not very sure whether it's a right approach, so please make a review.

micooz added a commit to micooz/node that referenced this issue Aug 24, 2017
When use TLSSocket, bufferSize is always +1, this affects users who
rely on this property to make judgments. This commit removed legacy
code and corrected the calculation of bufferSize.

Fixes: nodejs#15005
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Aug 24, 2017
apapirovski added a commit to apapirovski/node that referenced this issue Oct 18, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

Fixes: nodejs#15005
Refs: nodejs#15006
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
apapirovski added a commit to apapirovski/node that referenced this issue Oct 25, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs#15791
Fixes: nodejs#15005
Refs: nodejs#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs/node#15791
Fixes: nodejs/node#15005
Refs: nodejs/node#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs/node#15791
Fixes: nodejs/node#15005
Refs: nodejs/node#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
apapirovski added a commit to apapirovski/node that referenced this issue Dec 12, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

PR-URL: nodejs#15791
Fixes: nodejs#15005
Refs: nodejs#15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 20, 2017
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

Backport-PR-URL: #16420
PR-URL: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 2, 2018
Make writeQueueSize represent the actual size of the write queue
within the TLS socket. Add tls test to confirm that bufferSize
works as expected.

Backport-PR-URL: #16420
PR-URL: #15791
Fixes: #15005
Refs: #15006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants