Message-ID: From: "sehrope (@sehrope)" To: "pgjdbc/pgjdbc" Date: Tue, 26 May 2026 01:51:36 +0000 Subject: Re: [pgjdbc/pgjdbc] PR #4016: feat: harden protocol reader against desynced streams (#4015) In-Reply-To: References: List-Id: X-GitHub-Author-Login: sehrope X-GitHub-Comment-Id: 4538990636 X-GitHub-Comment-Type: issue_comment X-GitHub-Issue: 4016 X-GitHub-Repo: pgjdbc/pgjdbc X-GitHub-Type: comment X-GitHub-Url: https://github.com/pgjdbc/pgjdbc/pull/4016#issuecomment-4538990636 Content-Type: text/plain; charset=utf-8 This is a pretty invasive PR and I wouldn't want this merged without extensive review, understanding, and testing by someone else. For an idea of the size, the diffs for `PGStream` and `QueryExecutorImpl` are so large that GitHub does not even shown them by default. On the naming, I don't like "poison". It's not appropriate as that's not what that word means. I don't even think that aspect of the PR needs to exist. If there's a protocol violation for reading past the end of a message or a non-terminated string, just close the connection. We don't need to invent some new state for it. The connection would be garbage at that point anyway if it's going to be out of sync with the FEBE message flow. Just treat the same as we'd treat a socket error and close the connection. Overall I like the idea and I think that once we're comfortable with something like this, it should just be on. There's no point in having enablement modes or a "debug" mode. If it's actually catching a broken wire protocol it does not make sense to keep processing anything. We're either comfortable with the hardening changes or we're not. It's not a half measure situation. This goal may be better reached by splitting up the message tracking and more extensive checks. As a first step adding some plumbing for tracking the current message type + size. And propagating that through the code base. That's kind of done with the `pgStream.readMessageLength(...)`. As that's a known set of messages, it should take an enum, not an arbitrary string. Once that's in place, expanding things to include intra-message length checks when reading fields from the message (e.g, reading C-strings). That would make it a lot easier for someone to understand all the places that are being changed. Some of the other changes could go in on their own too. Like the buffer size changes to pgjdbc/src/main/java/org/postgresql/gss/GSSInputStream.java.