From 589d2c7fc304982d1f203965e8695564a6198e68 Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 28 Jan 2020 09:15:58 -0600 Subject: [PATCH 1/5] Close connection on SSL connection errors Fixes #2079 --- packages/pg/lib/connection.js | 6 ++++-- .../pg/test/integration/gh-issues/2079-tests.js | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 packages/pg/test/integration/gh-issues/2079-tests.js diff --git a/packages/pg/lib/connection.js b/packages/pg/lib/connection.js index 435c1a965..ee1642e00 100644 --- a/packages/pg/lib/connection.js +++ b/packages/pg/lib/connection.js @@ -85,11 +85,13 @@ Connection.prototype.connect = function (port, host) { this.stream.once('data', function (buffer) { var responseCode = buffer.toString('utf8') switch (responseCode) { - case 'N': // Server does not support SSL connections - return self.emit('error', new Error('The server does not support SSL connections')) case 'S': // Server supports SSL connections, continue with a secure connection break + case 'N': // Server does not support SSL connections + self.end() + return self.emit('error', new Error('The server does not support SSL connections')) default: // Any other response byte, including 'E' (ErrorResponse) indicating a server error + self.end() return self.emit('error', new Error('There was an error establishing an SSL connection')) } var tls = require('tls') diff --git a/packages/pg/test/integration/gh-issues/2079-tests.js b/packages/pg/test/integration/gh-issues/2079-tests.js new file mode 100644 index 000000000..660dabd11 --- /dev/null +++ b/packages/pg/test/integration/gh-issues/2079-tests.js @@ -0,0 +1,17 @@ + +"use strict" +var helper = require('./../test-helper') +var assert = require('assert') + +const suite = new helper.Suite() + +suite.test('SSL connection error allows event loop to exit', (done) => { + const client = new helper.pg.Client({ ssl: true }) + // since there was a connection error the client's socket should be closed + // and the event loop will have no refs and exit cleanly + client.connect((err) => { + assert(err instanceof Error) + done() + }) +}) + From a6ba5a3535173235b6de7768c5174ab03efe092a Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 28 Jan 2020 10:10:37 -0600 Subject: [PATCH 2/5] Fix test --- .../test/integration/gh-issues/2056-tests.js | 1 + .../test/integration/gh-issues/2079-tests.js | 37 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/pg/test/integration/gh-issues/2056-tests.js b/packages/pg/test/integration/gh-issues/2056-tests.js index f2912fc62..e025a1adc 100644 --- a/packages/pg/test/integration/gh-issues/2056-tests.js +++ b/packages/pg/test/integration/gh-issues/2056-tests.js @@ -5,6 +5,7 @@ var assert = require('assert') const suite = new helper.Suite() + suite.test('All queries should return a result array', (done) => { const client = new helper.pg.Client() client.connect() diff --git a/packages/pg/test/integration/gh-issues/2079-tests.js b/packages/pg/test/integration/gh-issues/2079-tests.js index 660dabd11..1b8b21876 100644 --- a/packages/pg/test/integration/gh-issues/2079-tests.js +++ b/packages/pg/test/integration/gh-issues/2079-tests.js @@ -5,8 +5,43 @@ var assert = require('assert') const suite = new helper.Suite() +// makes a backend server that responds with a non 'S' ssl response buffer +const makeTerminatingBackend = (code) => { + const { createServer } = require('net') + + const server = createServer((socket) => { + console.log('i got a socket') + const packet = Buffer.from(code, 'utf-8') + socket.write(packet) + // attach a listener so the socket can drain + socket.on('data', () => { + + }) + socket.on('close', () => { + server.close() + }) + }) + + server.listen() + const { port } = server.address() + return port +} + suite.test('SSL connection error allows event loop to exit', (done) => { - const client = new helper.pg.Client({ ssl: true }) + const port = makeTerminatingBackend('N') + const client = new helper.pg.Client({ ssl: true, port }) + // since there was a connection error the client's socket should be closed + // and the event loop will have no refs and exit cleanly + client.connect((err) => { + assert(err instanceof Error) + done() + }) +}) + + +suite.test('Non "S" response code allows event loop to exit', (done) => { + const port = makeTerminatingBackend('X') + const client = new helper.pg.Client({ ssl: true, port }) // since there was a connection error the client's socket should be closed // and the event loop will have no refs and exit cleanly client.connect((err) => { From 780fbad76577a3804f2d61897c89a2bae6bb6be0 Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 28 Jan 2020 10:12:57 -0600 Subject: [PATCH 3/5] Remove console.log --- packages/pg/test/integration/gh-issues/2079-tests.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/pg/test/integration/gh-issues/2079-tests.js b/packages/pg/test/integration/gh-issues/2079-tests.js index 1b8b21876..4877879af 100644 --- a/packages/pg/test/integration/gh-issues/2079-tests.js +++ b/packages/pg/test/integration/gh-issues/2079-tests.js @@ -10,7 +10,6 @@ const makeTerminatingBackend = (code) => { const { createServer } = require('net') const server = createServer((socket) => { - console.log('i got a socket') const packet = Buffer.from(code, 'utf-8') socket.write(packet) // attach a listener so the socket can drain From a20f8c1d4c78f081bde8298b10c54f31df6cd154 Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 28 Jan 2020 11:29:45 -0600 Subject: [PATCH 4/5] Fix tests, implement same behavior for native client --- packages/pg/lib/connection.js | 13 ++++++++++--- packages/pg/lib/native/client.js | 5 ++++- .../test/integration/gh-issues/2079-tests.js | 19 ++++++++++++------- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/packages/pg/lib/connection.js b/packages/pg/lib/connection.js index ee1642e00..9b4f14d1f 100644 --- a/packages/pg/lib/connection.js +++ b/packages/pg/lib/connection.js @@ -88,10 +88,10 @@ Connection.prototype.connect = function (port, host) { case 'S': // Server supports SSL connections, continue with a secure connection break case 'N': // Server does not support SSL connections - self.end() + self.stream.end() return self.emit('error', new Error('The server does not support SSL connections')) default: // Any other response byte, including 'E' (ErrorResponse) indicating a server error - self.end() + self.stream.end() return self.emit('error', new Error('There was an error establishing an SSL connection')) } var tls = require('tls') @@ -114,9 +114,16 @@ Connection.prototype.connect = function (port, host) { options.servername = host } self.stream = tls.connect(options) - self.attachListeners(self.stream) self.stream.on('error', reportStreamError) + // send SSLRequest packet + const buff = Buffer.alloc(8) + buff.writeUInt32BE(8) + buff.writeUInt32BE(80877103, 4) + self.stream.write(buff) + + self.attachListeners(self.stream) + self.emit('sslconnect') }) } diff --git a/packages/pg/lib/native/client.js b/packages/pg/lib/native/client.js index 6859bc2cc..581ef72d1 100644 --- a/packages/pg/lib/native/client.js +++ b/packages/pg/lib/native/client.js @@ -89,7 +89,10 @@ Client.prototype._connect = function (cb) { this.connectionParameters.getLibpqConnectionString(function (err, conString) { if (err) return cb(err) self.native.connect(conString, function (err) { - if (err) return cb(err) + if (err) { + self.native.end() + return cb(err) + } // set internal states to connected self._connected = true diff --git a/packages/pg/test/integration/gh-issues/2079-tests.js b/packages/pg/test/integration/gh-issues/2079-tests.js index 4877879af..bec8e481f 100644 --- a/packages/pg/test/integration/gh-issues/2079-tests.js +++ b/packages/pg/test/integration/gh-issues/2079-tests.js @@ -6,15 +6,20 @@ var assert = require('assert') const suite = new helper.Suite() // makes a backend server that responds with a non 'S' ssl response buffer -const makeTerminatingBackend = (code) => { +let makeTerminatingBackend = (byte) => { const { createServer } = require('net') const server = createServer((socket) => { - const packet = Buffer.from(code, 'utf-8') - socket.write(packet) // attach a listener so the socket can drain - socket.on('data', () => { - + // https://www.postgresql.org/docs/9.3/protocol-message-formats.html + socket.on('data', (buff) => { + const code = buff.readInt32BE(4) + // I don't see anything in the docs about 80877104 + // but libpq is sending it... + if (code === 80877103 || code === 80877104) { + const packet = Buffer.from(byte, 'utf-8') + socket.write(packet) + } }) socket.on('close', () => { server.close() @@ -28,7 +33,7 @@ const makeTerminatingBackend = (code) => { suite.test('SSL connection error allows event loop to exit', (done) => { const port = makeTerminatingBackend('N') - const client = new helper.pg.Client({ ssl: true, port }) + const client = new helper.pg.Client({ ssl: 'require', port }) // since there was a connection error the client's socket should be closed // and the event loop will have no refs and exit cleanly client.connect((err) => { @@ -40,7 +45,7 @@ suite.test('SSL connection error allows event loop to exit', (done) => { suite.test('Non "S" response code allows event loop to exit', (done) => { const port = makeTerminatingBackend('X') - const client = new helper.pg.Client({ ssl: true, port }) + const client = new helper.pg.Client({ ssl: 'require', port }) // since there was a connection error the client's socket should be closed // and the event loop will have no refs and exit cleanly client.connect((err) => { From b3c0874b58f02936afa0f0afd7507d0dba05200a Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 28 Jan 2020 12:18:11 -0600 Subject: [PATCH 5/5] Fix tests --- packages/pg-pool/test/error-handling.js | 8 ++++---- packages/pg/lib/connection.js | 8 +++++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/pg-pool/test/error-handling.js b/packages/pg-pool/test/error-handling.js index 72d97ede0..90de4ec41 100644 --- a/packages/pg-pool/test/error-handling.js +++ b/packages/pg-pool/test/error-handling.js @@ -211,14 +211,14 @@ describe('pool error handling', function () { const pool = new Pool({ max: 1, port: closeServer.address().port, host: 'localhost' }) pool.connect((err) => { expect(err).to.be.an(Error) - if (err.errno) { - expect(err.errno).to.be('ECONNRESET') + if (err.code) { + expect(err.code).to.be('ECONNRESET') } }) pool.connect((err) => { expect(err).to.be.an(Error) - if (err.errno) { - expect(err.errno).to.be('ECONNRESET') + if (err.code) { + expect(err.code).to.be('ECONNRESET') } closeServer.close(() => { pool.end(done) diff --git a/packages/pg/lib/connection.js b/packages/pg/lib/connection.js index 9b4f14d1f..6fa0696c9 100644 --- a/packages/pg/lib/connection.js +++ b/packages/pg/lib/connection.js @@ -120,7 +120,9 @@ Connection.prototype.connect = function (port, host) { const buff = Buffer.alloc(8) buff.writeUInt32BE(8) buff.writeUInt32BE(80877103, 4) - self.stream.write(buff) + if (self.stream.writable) { + self.stream.write(buff) + } self.attachListeners(self.stream) @@ -354,6 +356,10 @@ Connection.prototype.end = function () { // 0x58 = 'X' this.writer.add(emptyBuffer) this._ending = true + if (!this.stream.writable) { + this.stream.end() + return + } return this.stream.write(END_BUFFER, () => { this.stream.end() })