Skip to content

fix(parser): handle exceptions within handlePacket #3409

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.

Sign up for GitHub

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

Draft
avallete wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

fix(parser): handle exceptions within handlePacket #3409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2a168f
fix(parser): handle exceptions within handlePacket
avallete Mar 27, 2025
0089a69
chore: add test for parsing error handling
avallete Apr 2, 2025
65e7882
chore: setup e2e test for invalid packet error handling
avallete Apr 2, 2025
7886bcd
chore: skip e2e handling packet error test
avallete Apr 2, 2025
0020131
chore: fix typo
avallete Apr 2, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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
  • Loading branch information
@avallete
avallete committed Mar 27, 2025
commit c2a168f2626e7ecd527b7f29afc9941edb2f9996
26 changes: 26 additions & 0 deletions packages/pg-protocol/src/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
99 changes: 52 additions & 47 deletions packages/pg-protocol/src/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
MessageName,
AuthenticationMD5Password,
NoticeMessage,
ParserError,
} from './messages'
import { BufferReader } from './buffer-reader'

Expand Down Expand Up @@ -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 {
Copy link
Collaborator

@charmander charmander Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this try is too broad, and that every individual message handler should be able to handle arbitrary data gracefully, with try only being introduced at the most granular level possible when arbitrary exceptions can occur (usually because of user-provided functions).

It also erases important information about the original error since it only preserves its string representation instead of the complete object, but that’s more easily solved.
Copy link
Author

@avallete avallete Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment!

Yeah, I totally agree with [this comment](#2653 (comment))

“uncatchable errors are always no bueno”

sums it up well.

So to me, it makes sense to have a catch-all at the top level, just in case something slips through in a handler. That way, worst case, it's still catchable and doesn't crash the app. As any exception raised here will result in uncatchable error within pg side otherwise.

Then, we can still work toward handling better each possible exception within each of the packet handler. But this seems like a related but different concern that can be handled on it's own.

About this part:

Also, https://github.com/brianc/node-postgres/issues/2653’s case can be a query error, same as if a type parser failed. It doesn’t need to be treated like a protocol error.

Not totally sure I follow — by "query error", do you mean something like a SQL error (e.g. missing table)? If so, that should already be returned as a DatabaseError, no? This won't throw an exception but return the error to the caller. But I guess you’re referring to something else, maybe you can clarify?
Copy link
Collaborator

@charmander charmander Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A generic protocol error is fatal for the connection, since it’s in an unknown state after the failure. A row field that can’t be converted into a JavaScript string because it’s too big doesn’t have to be this type of error. The ideal fix to #2653 would handle this condition by exposing it as a query error instead of a protocol error.
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')
}
}

Expand Down