Only treat oversized ErrorResponse as plain text during startup#1326
Closed
momomuchu wants to merge 1 commit into
Closed
Only treat oversized ErrorResponse as plain text during startup#1326momomuchu wants to merge 1 commit into
momomuchu wants to merge 1 commit into
Conversation
recvMessage treated any ErrorResponse over MaxErrlen as a pre-protocol plain-text error (from PR lib#1249). libpq only applies that heuristic while awaiting the startup response; post-handshake an ErrorResponse is allowed to exceed MaxErrlen. Applying it in steady state garbled legitimate large errors and left the connection desynced: the message body and the following ReadyForQuery were never drained, so inProgress stayed stuck and the poisoned connection went back into the pool. Gate the check on a new startup bool, true only for conn.recv (used exclusively during the handshake). recv1Buf and the direct callers in copy.go and notify.go now pass false and parse large ErrorResponse messages normally. Fixes lib#1324
arp242
added a commit
that referenced
this pull request
Jul 11, 2026
…types #1248 is not entirely correct, as it should only apply on startup messages, as that check is done in PQconnectPoll(): if (beresp == PqMsg_ErrorResponse && (msgLength < 8 || msgLength > MAX_ERRLEN) We can re-use the txnStatus field to check if we're in the connection phase. In addition, in pqParseInput3(), it does limit the message length, but only for some respose types: #define VALID_LONG_MESSAGE_TYPE(id) \ ((id) == PqMsg_CopyData || \ (id) == PqMsg_DataRow || \ (id) == PqMsg_ErrorResponse || \ (id) == PqMsg_FunctionCallResponse || \ (id) == PqMsg_NoticeResponse || \ (id) == PqMsg_NotificationResponse || \ (id) == PqMsg_RowDescription) if (msgLength > 30000 && !VALID_LONG_MESSAGE_TYPE(id)) So check for that as well. Fixes #1324 Closes #1326
arp242
added a commit
that referenced
this pull request
Jul 11, 2026
…types #1248 is not entirely correct, as it should only apply on startup messages, as that check is done in PQconnectPoll(): if (beresp == PqMsg_ErrorResponse && (msgLength < 8 || msgLength > MAX_ERRLEN) We can re-use the txnStatus field to check if we're in the connection phase. In addition, in pqParseInput3(), it does limit the message length, but only for some respose types: #define VALID_LONG_MESSAGE_TYPE(id) \ ((id) == PqMsg_CopyData || \ (id) == PqMsg_DataRow || \ (id) == PqMsg_ErrorResponse || \ (id) == PqMsg_FunctionCallResponse || \ (id) == PqMsg_NoticeResponse || \ (id) == PqMsg_NotificationResponse || \ (id) == PqMsg_RowDescription) if (msgLength > 30000 && !VALID_LONG_MESSAGE_TYPE(id)) So check for that as well. Fixes #1324 Closes #1326
arp242
added a commit
that referenced
this pull request
Jul 11, 2026
…types #1248 is not entirely correct, as it should only apply on startup messages, as that check is done in PQconnectPoll(): if (beresp == PqMsg_ErrorResponse && (msgLength < 8 || msgLength > MAX_ERRLEN) We can re-use the txnStatus field to check if we're in the connection phase. In addition, in pqParseInput3(), it does limit the message length, but only for some respose types: #define VALID_LONG_MESSAGE_TYPE(id) \ ((id) == PqMsg_CopyData || \ (id) == PqMsg_DataRow || \ (id) == PqMsg_ErrorResponse || \ (id) == PqMsg_FunctionCallResponse || \ (id) == PqMsg_NoticeResponse || \ (id) == PqMsg_NotificationResponse || \ (id) == PqMsg_RowDescription) if (msgLength > 30000 && !VALID_LONG_MESSAGE_TYPE(id)) So check for that as well. Fixes #1324 Closes #1326
arp242
added a commit
that referenced
this pull request
Jul 11, 2026
…types #1248 is not entirely correct, as it should only apply on startup messages, as that check is done in PQconnectPoll(): if (beresp == PqMsg_ErrorResponse && (msgLength < 8 || msgLength > MAX_ERRLEN) We can re-use the txnStatus field to check if we're in the connection phase. In addition, in pqParseInput3(), it does limit the message length, but only for some respose types: #define VALID_LONG_MESSAGE_TYPE(id) \ ((id) == PqMsg_CopyData || \ (id) == PqMsg_DataRow || \ (id) == PqMsg_ErrorResponse || \ (id) == PqMsg_FunctionCallResponse || \ (id) == PqMsg_NoticeResponse || \ (id) == PqMsg_NotificationResponse || \ (id) == PqMsg_RowDescription) if (msgLength > 30000 && !VALID_LONG_MESSAGE_TYPE(id)) So check for that as well. Fixes #1324 Closes #1326
Collaborator
|
I fixed it via #1327, which more accurately copies what libpq does wrt. message sizes. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1324
Since #1249, recvMessage treats any ErrorResponse over MaxErrlen (30000) as a pre-protocol plain-text error. libpq only applies that heuristic while awaiting the startup response (fe-connect.c, PQconnectPoll); after the handshake an ErrorResponse is explicitly allowed to exceed 30000 bytes (fe-protocol3.c, VALID_LONG_MESSAGE_TYPE).
Applying it in steady state does two things to a legitimate large error, e.g.
RAISE EXCEPTION '%', repeat('x', 40000): the error comes back garbled (the raw length bytes plus the first field instead of the parsed message), and the connection is left desynced, since the early return never drains the message body or the trailing ReadyForQuery. inProgress stays stuck, the conn is not marked bad, and the pool hands it back out, so the next query fails with "there is already a query being processed on this connection" (see the second comment on #1324 for the full trace).This gates the heuristic on a new startup parameter to recvMessage, true only from conn.recv, which is only used during the startup/auth sequence. recv1Buf and the direct callers in copy.go and notify.go pass false and parse large ErrorResponses normally, matching libpq. The pre-handshake "could not fork" case from #1249 keeps working as before.
This is unrelated to #1320/#1321, which is an EOF-handling bug in the extended-protocol cleanup paths; same end symptom, different cause and code path.
Test uses pqtest.Fake to send a MaxErrlen+10000 ErrorResponse and asserts the message and SQLSTATE parse intact and that a second query on the same pooled connection still works.