From c2a168f2626e7ecd527b7f29afc9941edb2f9996 Mon Sep 17 00:00:00 2001 From: avallete Date: Thu, 27 Mar 2025 23:25:24 +0100 Subject: [PATCH 1/5] fix(parser): handle exceptions within handlePacket Exception occuring within the handlePacket will bubble up to pg as uncatchables exceptions errors. Adding a try/catch and returning an handled error allow the caller program to catch such exceptions and decide what to do with them. Fixes #2653 --- packages/pg-protocol/src/messages.ts | 26 ++++++++ packages/pg-protocol/src/parser.ts | 99 +++++++++++++++------------- 2 files changed, 78 insertions(+), 47 deletions(-) diff --git a/packages/pg-protocol/src/messages.ts b/packages/pg-protocol/src/messages.ts index c3fbbdd9b..844fd7b81 100644 --- a/packages/pg-protocol/src/messages.ts +++ b/packages/pg-protocol/src/messages.ts @@ -120,6 +120,32 @@ export class DatabaseError extends Error implements NoticeOrError { } } +export class ParserError extends Error implements NoticeOrError { + public severity: string | undefined + public code: string | undefined + public detail: string | undefined + public hint: string | undefined + public position: string | undefined + public internalPosition: string | undefined + public internalQuery: string | undefined + public where: string | undefined + public schema: string | undefined + public table: string | undefined + public column: string | undefined + public dataType: string | undefined + public constraint: string | undefined + public file: string | undefined + public line: string | undefined + public routine: string | undefined + constructor( + message: string, + public readonly length: number, + public readonly name: MessageName + ) { + super(message) + } +} + export class CopyDataMessage { public readonly name = 'copyData' constructor( diff --git a/packages/pg-protocol/src/parser.ts b/packages/pg-protocol/src/parser.ts index 3b901aefe..d119b0c7a 100644 --- a/packages/pg-protocol/src/parser.ts +++ b/packages/pg-protocol/src/parser.ts @@ -25,6 +25,7 @@ import { MessageName, AuthenticationMD5Password, NoticeMessage, + ParserError, } from './messages' import { BufferReader } from './buffer-reader' @@ -152,53 +153,57 @@ export class Parser { } private handlePacket(offset: number, code: number, length: number, bytes: Buffer): BackendMessage { - switch (code) { - case MessageCodes.BindComplete: - return bindComplete - case MessageCodes.ParseComplete: - return parseComplete - case MessageCodes.CloseComplete: - return closeComplete - case MessageCodes.NoData: - return noData - case MessageCodes.PortalSuspended: - return portalSuspended - case MessageCodes.CopyDone: - return copyDone - case MessageCodes.ReplicationStart: - return replicationStart - case MessageCodes.EmptyQuery: - return emptyQuery - case MessageCodes.DataRow: - return this.parseDataRowMessage(offset, length, bytes) - case MessageCodes.CommandComplete: - return this.parseCommandCompleteMessage(offset, length, bytes) - case MessageCodes.ReadyForQuery: - return this.parseReadyForQueryMessage(offset, length, bytes) - case MessageCodes.NotificationResponse: - return this.parseNotificationMessage(offset, length, bytes) - case MessageCodes.AuthenticationResponse: - return this.parseAuthenticationResponse(offset, length, bytes) - case MessageCodes.ParameterStatus: - return this.parseParameterStatusMessage(offset, length, bytes) - case MessageCodes.BackendKeyData: - return this.parseBackendKeyData(offset, length, bytes) - case MessageCodes.ErrorMessage: - return this.parseErrorMessage(offset, length, bytes, 'error') - case MessageCodes.NoticeMessage: - return this.parseErrorMessage(offset, length, bytes, 'notice') - case MessageCodes.RowDescriptionMessage: - return this.parseRowDescriptionMessage(offset, length, bytes) - case MessageCodes.ParameterDescriptionMessage: - return this.parseParameterDescriptionMessage(offset, length, bytes) - case MessageCodes.CopyIn: - return this.parseCopyInMessage(offset, length, bytes) - case MessageCodes.CopyOut: - return this.parseCopyOutMessage(offset, length, bytes) - case MessageCodes.CopyData: - return this.parseCopyData(offset, length, bytes) - default: - return new DatabaseError('received invalid response: ' + code.toString(16), length, 'error') + try { + switch (code) { + case MessageCodes.BindComplete: + return bindComplete + case MessageCodes.ParseComplete: + return parseComplete + case MessageCodes.CloseComplete: + return closeComplete + case MessageCodes.NoData: + return noData + case MessageCodes.PortalSuspended: + return portalSuspended + case MessageCodes.CopyDone: + return copyDone + case MessageCodes.ReplicationStart: + return replicationStart + case MessageCodes.EmptyQuery: + return emptyQuery + case MessageCodes.DataRow: + return this.parseDataRowMessage(offset, length, bytes) + case MessageCodes.CommandComplete: + return this.parseCommandCompleteMessage(offset, length, bytes) + case MessageCodes.ReadyForQuery: + return this.parseReadyForQueryMessage(offset, length, bytes) + case MessageCodes.NotificationResponse: + return this.parseNotificationMessage(offset, length, bytes) + case MessageCodes.AuthenticationResponse: + return this.parseAuthenticationResponse(offset, length, bytes) + case MessageCodes.ParameterStatus: + return this.parseParameterStatusMessage(offset, length, bytes) + case MessageCodes.BackendKeyData: + return this.parseBackendKeyData(offset, length, bytes) + case MessageCodes.ErrorMessage: + return this.parseErrorMessage(offset, length, bytes, 'error') + case MessageCodes.NoticeMessage: + return this.parseErrorMessage(offset, length, bytes, 'notice') + case MessageCodes.RowDescriptionMessage: + return this.parseRowDescriptionMessage(offset, length, bytes) + case MessageCodes.ParameterDescriptionMessage: + return this.parseParameterDescriptionMessage(offset, length, bytes) + case MessageCodes.CopyIn: + return this.parseCopyInMessage(offset, length, bytes) + case MessageCodes.CopyOut: + return this.parseCopyOutMessage(offset, length, bytes) + case MessageCodes.CopyData: + return this.parseCopyData(offset, length, bytes) + default: + return new DatabaseError('received invalid response: ' + code.toString(16), length, 'error') + } + } catch (error) { + return new ParserError(`unexpected error handling packet: ${error}`, length, 'error') } } From 0089a697c241d11b389da1a27eb5c86150111006 Mon Sep 17 00:00:00 2001 From: avallete Date: Wed, 2 Apr 2025 18:21:15 +0200 Subject: [PATCH 2/5] chore: add test for parsing error handling --- .../pg-protocol/src/inbound-parser.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/pg-protocol/src/inbound-parser.test.ts b/packages/pg-protocol/src/inbound-parser.test.ts index 345d1ca8a..e3a012f0e 100644 --- a/packages/pg-protocol/src/inbound-parser.test.ts +++ b/packages/pg-protocol/src/inbound-parser.test.ts @@ -573,4 +573,34 @@ describe('PgPacketStream', function () { }) }) }) + + describe('error handling', () => { + it('should handle unexpected errors in handlePacket', async () => { + // Create a buffer with a valid row description code (0x54) and valid length + // but invalid field data that will cause a parsing error + const malformedBuffer = Buffer.from([ + 0x54, // RowDescription message code + 0x00, + 0x00, + 0x00, + 0x0b, // length (11 bytes) + 0x00, + 0x01, // field count (1) + 0x00, + 0x00, + 0x00, + 0x00, // invalid field data + 0x00, + 0x00, + 0x00, + 0x00, // invalid field data + ]) + + const messages = await parseBuffers([malformedBuffer]) + assert.strictEqual(messages.length, 1) + assert.strictEqual(messages[0].name, 'error') + assert(messages[0] instanceof Error) + assert(messages[0].message.includes('unexpected error handling packet')) + }) + }) }) From 65e7882e39b27b9b597c1bec4d0e58d9f15e6d79 Mon Sep 17 00:00:00 2001 From: avallete Date: Wed, 2 Apr 2025 18:53:50 +0200 Subject: [PATCH 3/5] chore: setup e2e test for invalid packet error handling --- packages/pg/package.json | 2 +- .../connection-pool/error-tests.js | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/pg/package.json b/packages/pg/package.json index 48d1b55f8..a0845192d 100644 --- a/packages/pg/package.json +++ b/packages/pg/package.json @@ -22,7 +22,7 @@ "dependencies": { "pg-connection-string": "^2.7.0", "pg-pool": "^3.8.0", - "pg-protocol": "^1.8.0", + "pg-protocol": "file:../pg-protocol", "pg-types": "^2.1.0", "pgpass": "1.x" }, diff --git a/packages/pg/test/integration/connection-pool/error-tests.js b/packages/pg/test/integration/connection-pool/error-tests.js index 9f20aa4e6..a87262bc0 100644 --- a/packages/pg/test/integration/connection-pool/error-tests.js +++ b/packages/pg/test/integration/connection-pool/error-tests.js @@ -164,3 +164,31 @@ suite.test('handles socket error during pool.query and destroys it immediately', }, 100) } }) + +suite.test('unexpected handling packet error emit catchable errors', () => { + const pool = new pg.Pool() + pool.connect( + assert.success((client, done) => { + client.once( + 'error', + // After unexpected packed error, the client will receive an unexpected commandComplete message from backend + assert.calls((err) => { + assert.equal(`${err}`, 'Error: Received unexpected commandComplete message from backend.') + done() + }) + ) + client.query( + // Retrieve a field which lenght cannot be handled by JS (+700Mo string) + `SELECT repeat('A', 536870889)`, + assert.calls((err) => { + if (helper.args.native) { + assert.ok(err) + } else { + assert.equal(err.message, 'unexpected error handling packet: Error: Cannot create a string longer than 0x1fffffe8 characters') + assert.equal(err.length, 536870899) + } + }) + ) + }) + ) +}) From 7886bcd7f7bc70507d9ce8bf23a715ac1b1a2b7d Mon Sep 17 00:00:00 2001 From: avallete Date: Wed, 2 Apr 2025 18:55:38 +0200 Subject: [PATCH 4/5] chore: skip e2e handling packet error test --- packages/pg/package.json | 2 +- .../connection-pool/error-tests.js | 54 +++++++++---------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/packages/pg/package.json b/packages/pg/package.json index a0845192d..48d1b55f8 100644 --- a/packages/pg/package.json +++ b/packages/pg/package.json @@ -22,7 +22,7 @@ "dependencies": { "pg-connection-string": "^2.7.0", "pg-pool": "^3.8.0", - "pg-protocol": "file:../pg-protocol", + "pg-protocol": "^1.8.0", "pg-types": "^2.1.0", "pgpass": "1.x" }, diff --git a/packages/pg/test/integration/connection-pool/error-tests.js b/packages/pg/test/integration/connection-pool/error-tests.js index a87262bc0..84bd66a0c 100644 --- a/packages/pg/test/integration/connection-pool/error-tests.js +++ b/packages/pg/test/integration/connection-pool/error-tests.js @@ -165,30 +165,30 @@ suite.test('handles socket error during pool.query and destroys it immediately', } }) -suite.test('unexpected handling packet error emit catchable errors', () => { - const pool = new pg.Pool() - pool.connect( - assert.success((client, done) => { - client.once( - 'error', - // After unexpected packed error, the client will receive an unexpected commandComplete message from backend - assert.calls((err) => { - assert.equal(`${err}`, 'Error: Received unexpected commandComplete message from backend.') - done() - }) - ) - client.query( - // Retrieve a field which lenght cannot be handled by JS (+700Mo string) - `SELECT repeat('A', 536870889)`, - assert.calls((err) => { - if (helper.args.native) { - assert.ok(err) - } else { - assert.equal(err.message, 'unexpected error handling packet: Error: Cannot create a string longer than 0x1fffffe8 characters') - assert.equal(err.length, 536870899) - } - }) - ) - }) - ) -}) +// suite.test('unexpected handling packet error emit catchable errors', () => { +// const pool = new pg.Pool() +// pool.connect( +// assert.success((client, done) => { +// client.once( +// 'error', +// // After unexpected packed error, the client will receive an unexpected commandComplete message from backend +// assert.calls((err) => { +// assert.equal(`${err}`, 'Error: Received unexpected commandComplete message from backend.') +// done() +// }) +// ) +// client.query( +// // Retrieve a field which lenght cannot be handled by JS (+700Mo string) +// `SELECT repeat('A', 536870889)`, +// assert.calls((err) => { +// if (helper.args.native) { +// assert.ok(err) +// } else { +// assert.equal(err.message, 'unexpected error handling packet: Error: Cannot create a string longer than 0x1fffffe8 characters') +// assert.equal(err.length, 536870899) +// } +// }) +// ) +// }) +// ) +// }) From 00201310edd74a0656813697324ff2ecefb768d4 Mon Sep 17 00:00:00 2001 From: avallete Date: Wed, 2 Apr 2025 16:59:25 +0000 Subject: [PATCH 5/5] chore: fix typo --- packages/pg/test/integration/connection-pool/error-tests.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pg/test/integration/connection-pool/error-tests.js b/packages/pg/test/integration/connection-pool/error-tests.js index 84bd66a0c..1aecaaea7 100644 --- a/packages/pg/test/integration/connection-pool/error-tests.js +++ b/packages/pg/test/integration/connection-pool/error-tests.js @@ -178,7 +178,7 @@ suite.test('handles socket error during pool.query and destroys it immediately', // }) // ) // client.query( -// // Retrieve a field which lenght cannot be handled by JS (+700Mo string) +// // Retrieve a field which length cannot be handled by JS // `SELECT repeat('A', 536870889)`, // assert.calls((err) => { // if (helper.args.native) {