Reject malformed element counts in message decoders - #451
Open
gaoflow wants to merge 3 commits into
Open
Conversation
Several decoders read a repeated-element count as a signed i16/i32 and pass it to Vec::with_capacity before reading any element. A negative count (0xffff read as i16 = -1) sign-extends to usize::MAX and panics with "capacity overflow", so a short crafted message crashes the peer decoding it (the shipped client/proxy on backend messages, the server on Bind). sunng87#192 fixed this for Parse, Bind and ParameterDescription by widening the count to u16 but left RowDescription, the three Copy*Response messages, NegotiateProtocolVersion, and Bind's result_column_format_codes. Read those counts as unsigned and route them through a shared codec::read_count that rejects a count exceeding the bytes remaining, turning a malformed message into a decode error instead of a panic.
sunng87
reviewed
Jul 29, 2026
sunng87
reviewed
Jul 31, 2026
sunng87
reviewed
Aug 1, 2026
Author
|
Updated in 7eff654: CopyInResponse now keeps the wire count as |
Counts on the wire are element counts, not byte counts: Bind's result format codes are Int16s, so a count that fits the remaining bytes can still overrun the buffer when read as 2-byte elements. ensure_count now takes the per-element size and rejects count * size > remaining.
Author
|
Updated in e68abf0: CopyInResponse now keeps the wire count as |
gaoflow
force-pushed
the
fix-signed-count-capacity-overflow
branch
from
August 1, 2026 13:08
e6dac70 to
e68abf0
Compare
sunng87
reviewed
Aug 1, 2026
| fn decode_body(buf: &mut BytesMut, _len: usize, _ctx: &DecodeContext) -> PgWireResult<Self> { | ||
| let format = buf.get_i8(); | ||
| let columns = buf.get_i16(); | ||
| let columns = buf.get_u16(); |
Owner
There was a problem hiding this comment.
please follow the same pattern for this and all count fields below
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.
Several message decoders read a repeated-element count as a signed
i16/i32and hand it toVec::with_capacitybefore reading any element. A negative count (0xffffasi16= -1) sign-extends tousize::MAXand panics withcapacity overflow, so a ~7-byte crafted message crashes whoever decodes it — the shipped client/proxy on a backend message, or the server onBind.Repro (debug and release):
PgWireBackendMessage::decode(&mut BytesMut::from(&b"T\0\0\0\x06\xff\xff"[..]), &ctx)→capacity overflow.#192 fixed this for
Parse,BindandParameterDescriptionby widening the count tou16but stopped short of the siblings that share the shape. This completes it for the six missed:RowDescription,CopyInResponse/CopyOutResponse/CopyBothResponse,NegotiateProtocolVersion(re-introduced in #281), andBind'sresult_column_format_codes(#192 widened the other two counts in that same function, not this third).The counts are read unsigned and routed through one shared
codec::read_countthat rejects a count larger than the remaining bytes, turning a malformed message into a decodeErr. A single helper also keeps the pattern from silently reappearing as it did in #281; and reading unsigned alone isn't enough for thei32option count, whereu32::MAXstill forces a huge allocation, so the bound is applied uniformly. The four counts #192 already widened are left untouched.Tests exercise all six sites through the public
decodeAPI, plus zero-count and well-formed positive-count controls.