Skip to content

feat(relay): Add a bounded deserializer for (prost) protobuffers - #6295

Open
klochek wants to merge 3 commits into
masterfrom
christopherklochek/relay_serializer_proto
Open

feat(relay): Add a bounded deserializer for (prost) protobuffers#6295
klochek wants to merge 3 commits into
masterfrom
christopherklochek/relay_serializer_proto

Conversation

@klochek

@klochek klochek commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

This is implemented in two parts. The first is in tools/proto-descriptors, and is an offline tool to generate "descriptor" files from input .proto files. These generated files give us the metadata necessary to make decisions during the scanning of the wire payload about what constitutes a nested message and what does not. The second part is in relay-serializers, which does the actual payload scanning. The scanner examines the payload as a pre-processing step to actual deserialization, and uses the descriptor files to understand the parts of a proto payload. In particular, we need to be able to distinguish between a string/vec, and a list of messages, as the former will be deserialized in one "operation" (and the accounting for it will already be done, as part of limiting the actual payload size,) while the latter needs to be treated as N operations (to prevent quadratic blow-outs, etc.)

klochek added 3 commits August 7, 2026 08:18
This is implemented in two parts.  The first is in tools/proto-descriptors, and is an offline tool to generate "descriptor" files from input .proto files.  These generated files give us the metadata necessary to make decisions during the scanning of the wire payload about what constitutes a nested message and what does not.  The second part is in relay-serializers, which does the actual payload scanning.  The scanner examines the payload as a pre-processing step to actual deserialization, and uses the descriptor files to understand the parts of a proto payload.  In particular, we need to be able to distinguish between a string/vec, and a list of messages, as the former will be deserialized in one "operation" (and the accounting for it will already be done, as part of limiting the actual payload size,) while the latter needs to be treated as N operations (to prevent quadratic blow-outs, etc.)
@klochek
klochek requested a review from a team as a code owner August 7, 2026 12:20
for message in &file.message_type {
collect(&prefix, message, &mut messages)?;

if file.name() == root_file {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The path comparison at file.name() == root_file is not platform-agnostic and will fail silently on Windows if the path contains backslashes, leading to incomplete code generation.
Severity: LOW

Suggested Fix

Normalize the paths before comparison to ensure they use a consistent format. Convert both file.name() and root_file to a canonical representation, for example by using a library like path-clean or by manually replacing path separators, before checking for equality.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: tools/proto-descriptors/src/main.rs#L119

Potential issue: The comparison `file.name() == root_file` is not platform-agnostic. The
`file.name()` from `protoc` is a string that always uses forward slashes, while
`root_file` is a `Path` that may use backslashes on Windows. This discrepancy causes the
equality check to fail silently on Windows when paths with backslashes are used. As a
result, `root_types` remains empty, leading to incomplete code generation (missing `impl
Decodable` blocks) without any error or warning.

Did we get this right? 👍 / 👎 to inform future reviews.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 61cd22e. Configure here.


fn walk<'a>(messages: BTreeMap<String, &'a DescriptorProto>) -> Result<Vec<ProcessedMessage<'a>>> {
let mut processed = Vec::new();
let mut queue: Vec<String> = messages.keys().map(|k| k.to_owned()).collect();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Traversal includes every imported message

Medium Severity

walk seeds its queue with every message from --include_imports, rather than only root messages and their transitive dependencies. Unrelated imported maps, groups, or identifier collisions can therefore make descriptor generation fail for an otherwise supported schema.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 61cd22e. Configure here.


if file.name() == root_file {
root_types.push(message.name().to_owned());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Canonical paths can erase all roots

Medium Severity

FileDescriptorProto.name is canonicalized relative to proto_root, but it is compared directly with the CLI's root_file. Passing an absolute or proto_root-prefixed path leaves root_types empty, silently generating no Decodable implementations.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 61cd22e. Configure here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant