Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ tokio-util = { version = "0.7", features = ["rt"] }
futures = "0.3"
async-trait = "0.1"
bytes = "1"
byteorder = "1"
Comment thread
leynos marked this conversation as resolved.
log = "0.4"
dashmap = "5"

Expand Down
77 changes: 42 additions & 35 deletions docs/rust-binary-router-library-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -1123,50 +1123,57 @@ examples are invaluable. They make the abstract design tangible and showcase how
2. **Frame Processor Implementation** (Simple Length-Prefixed Framing using
`tokio-util`):

```rust
// Crate: my_frame_processor.rs
use bytes::{BytesMut, Buf, BufMut};
use tokio_util::codec::{Encoder, Decoder};
use std::io;
```rust
// Crate: my_frame_processor.rs
use bytes::{BytesMut, Buf, BufMut};
use tokio_util::codec::{Decoder, Encoder};
use byteorder::{BigEndian, ReadBytesExt};
use std::io;

pub struct LengthPrefixedCodec;
const MAX_FRAME_LEN: usize = 16 * 1024 * 1024; // 16 MiB upper limit

impl Decoder for LengthPrefixedCodec {
type Item = BytesMut; // Raw frame payload
type Error = io::Error;
pub struct LengthPrefixedCodec;

fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, Self::Error> {
if src.len() < 4 { return Ok(None); } // Not enough data for length prefix
impl Decoder for LengthPrefixedCodec {
type Item = BytesMut; // Raw frame payload
type Error = io::Error;

let mut length_bytes = [0u8; 4];
length_bytes.copy_from_slice(&src[..4]);
let length = u32::from_be_bytes(length_bytes) as usize;
fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, Self::Error> {
if src.len() < 4 { return Ok(None); } // Not enough data for length prefix

if src.len() < 4 + length {
src.reserve(4 + length - src.len());
return Ok(None); // Not enough data for full frame
}
let length = (&src[..4])
.read_u32::<BigEndian>()
.expect("slice length checked") as usize;

src.advance(4); // Consume length prefix
Ok(Some(src.split_to(length)))
}
}
if length > MAX_FRAME_LEN {
return Err(io::Error::new(io::ErrorKind::InvalidInput, "frame too large"));
}

impl<T: AsRef<[u8]>> Encoder<T> for LengthPrefixedCodec {
type Error = io::Error;
if src.len() < 4 + length {
src.reserve(4 + length - src.len());
return Ok(None); // Not enough data for full frame
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

fn encode(&mut self, item: T, dst: &mut BytesMut) -> Result<(), Self::Error> {
let data = item.as_ref();
dst.reserve(4 + data.len());
dst.put_u32(data.len() as u32);
dst.put_slice(data);
Ok(())
}
}
```
src.advance(4); // Consume length prefix
Ok(Some(src.split_to(length)))
}
}

impl<T: AsRef<[u8]>> Encoder<T> for LengthPrefixedCodec {
type Error = io::Error;

fn encode(&mut self, item: T, dst: &mut BytesMut) -> Result<(), Self::Error> {
let data = item.as_ref();
dst.reserve(4 + data.len());
dst.put_u32(data.len() as u32);
dst.put_slice(data);
Ok(())
Comment on lines +1165 to +1170
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jul 4, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Encode path silently truncates frames > 4 GiB.

data.len() as u32 wraps for payloads above u32::MAX, producing corrupt output. At minimum, validate length and return an error.

-    dst.put_u32(data.len() as u32);
+    if data.len() > u32::MAX as usize {
+        return Err(io::Error::new(
+            io::ErrorKind::InvalidInput,
+            "payload exceeds 4 GiB limit",
+        ));
+    }
+    dst.put_u32(data.len() as u32);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn encode(&mut self, item: T, dst: &mut BytesMut) -> Result<(), Self::Error> {
let data = item.as_ref();
dst.reserve(4 + data.len());
dst.put_u32(data.len() as u32);
dst.put_slice(data);
Ok(())
fn encode(&mut self, item: T, dst: &mut BytesMut) -> Result<(), Self::Error> {
let data = item.as_ref();
dst.reserve(4 + data.len());
if data.len() > u32::MAX as usize {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"payload exceeds 4 GiB limit",
));
}
dst.put_u32(data.len() as u32);
dst.put_slice(data);
Ok(())
🤖 Prompt for AI Agents
In docs/rust-binary-router-library-design.md around lines 1159 to 1164, the
encode function casts data length to u32 without checking if it exceeds
u32::MAX, causing silent truncation for frames larger than 4 GiB. Fix this by
adding a validation step before casting: if data.len() is greater than u32::MAX,
return an error instead of proceeding. This prevents corrupt output by ensuring
only valid frame sizes are encoded.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@coderabbitai new GitHub issue for this, please

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

I'll create a GitHub issue for the documentation bug you've identified.

@leynos I've created a GitHub issue to track the documentation bug regarding the potential overflow in the frame encoder example. The issue includes details about the problem, the suggested fix, and references back to this PR and comment.


🏁 Script executed:

#!/bin/bash
# Create GitHub issue for the documentation overflow bug
gh issue create \
  --title "Documentation: Frame encoder silently truncates payloads > 4 GiB" \
  --body "## Description

The example code in \`docs/rust-binary-router-library-design.md\` for the \`LengthPrefixedCodec\` encoder has a potential overflow bug that could cause silent truncation for large payloads.

## Problem

In the \`encode\` function, the line:
\`\`\`rust
dst.put_u32(data.len() as u32);
\`\`\`

Casts \`data.len()\` to \`u32\` without validation. For payloads larger than 4 GiB (\`u32::MAX\`), this will wrap around and produce corrupt output with an incorrect length prefix.

## Solution

Add validation before the cast:
\`\`\`rust
if data.len() > u32::MAX as usize {
    return Err(io::Error::new(
        io::ErrorKind::InvalidInput,
        \"payload exceeds 4 GiB limit\",
    ));
}
dst.put_u32(data.len() as u32);
\`\`\`

## Context

- **PR**: https://github.com/leynos/wireframe/pull/162
- **Comment**: https://github.com/leynos/wireframe/pull/162#discussion_r2186125248
- **Requested by**: @leynos

While this is example code in documentation rather than production code, it's important to demonstrate best practices to avoid misleading developers who might copy this pattern." \
  --assignee leynos

Length of output: 1241


Perfect! I've successfully created GitHub issue #174 for the documentation bug. You can view it here: #174

The issue includes:

  • Clear description of the overflow problem in the example code
  • The specific problematic line and suggested fix
  • Links back to this PR and comment thread
  • Assignment to you as requested

This will help track the need to fix the documentation example to demonstrate proper bounds checking for large payload sizes.

}
}
```

(Note: "wireframe" would abstract the direct use of `Encoder`/`Decoder`
behind its own `FrameProcessor` trait or provide helpers.)
(Note: "wireframe" would abstract the direct use of `Encoder`/`Decoder` behind
its own `FrameProcessor` trait or provide helpers.)

1. **Server Setup and Handler**:

Expand Down
Loading
Loading