Skip to content

fix: make encode_item panic/error-safe so it never exposes uninitialized header bytes#2742

Open
mvanhorn wants to merge 1 commit into
grpc:masterfrom
mvanhorn:fix/2720-encode-item-panic-safety
Open

fix: make encode_item panic/error-safe so it never exposes uninitialized header bytes#2742
mvanhorn wants to merge 1 commit into
grpc:masterfrom
mvanhorn:fix/2720-encode-item-panic-safety

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Motivation

encode_item in tonic/src/codec/encode.rs reserves a 5-byte frame header via buf.reserve(HEADER_SIZE) and advances the buffer length over those bytes before the message payload is encoded. If encoding then fails, either by returning an error or by panicking inside a user-supplied Encoder/compression codec, the BytesMut is left with its length advanced over header bytes that were never written. Those bytes hold whatever was previously in the buffer's spare capacity. On the error path the frame is abandoned; on an unwinding panic that a caller catches (or that crosses a catch_unwind boundary in a surrounding runtime) the buffer can subsequently be observed with a partially-formed frame whose header is uninitialized leftover memory.

Closes #2720.

Solution

Wrap the header reservation and payload encoding in a drop guard that records the buffer's original length up front and truncates back to it unless encode_item reaches its normal return. On the success path the guard is disarmed and the fully-framed buffer is preserved; on any early return or unwinding panic the guard's Drop restores the buffer to exactly the bytes that were present before the call, so no half-written frame or uninitialized header is ever left behind.

Regression tests cover the five relevant paths: successful framing (header + payload written), encoder error, finish_encoding/framing error raised late, encoder panic (buffer restored after unwinding), and compressed-encoder failure with the gzip feature enabled. cargo test -p tonic codec::encode passes with default features (4 tests) and with --features gzip (5 tests); cargo fmt --check and git diff --check are clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tonic's encode_item function is not exception safe, leading to use of uninitialized memory

1 participant