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
104 changes: 94 additions & 10 deletions Cargo.lock

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

34 changes: 23 additions & 11 deletions minecraft/src/serialization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,19 @@ use crate::{
const SEGMENT_BITS: u32 = 0x7F;
const CONTINUE_BIT: u32 = 0x80;

/// Returns the number of bytes a VarInt value occupies when encoded.
fn varint_size(value: i32) -> usize {
let mut value = value as u32;
let mut size = 0;
loop {
size += 1;
if (value & !SEGMENT_BITS) == 0 {
return size;
}
value >>= 7;
}
}

Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The new varint_size function calculates the encoded size of a VarInt, which is critical for the packet length fix. However, there are no tests verifying this function works correctly. Given that the minecraft crate has existing test infrastructure (tests/field_types.rs, tests/truncate_to_zero.rs), consider adding test cases that verify varint_size returns the correct values for edge cases like 0, 127, 128, -1, and maximum/minimum i32 values. This would help prevent regressions in this critical protocol parsing logic.

Suggested change
#[cfg(test)]
mod tests {
use super::varint_size;
#[test]
fn test_varint_size_edge_cases() {
// Basic small values
assert_eq!(varint_size(0), 1);
assert_eq!(varint_size(1), 1);
assert_eq!(varint_size(127), 1);
// Boundary where an extra byte is needed
assert_eq!(varint_size(128), 2);
// Negative values and extremes of i32 range
assert_eq!(varint_size(-1), 5);
assert_eq!(varint_size(i32::MAX), 5);
assert_eq!(varint_size(i32::MIN), 5);
}
}

Copilot uses AI. Check for mistakes.
#[derive(Debug, PartialEq)]
pub enum ReadingError {
Insufficient,
Expand Down Expand Up @@ -93,7 +106,7 @@ impl<RW: AsyncRead + AsyncWrite + Unpin> MinecraftStream<RW> {
}

pub fn data_len(&self) -> usize {
self.free - self.position + 1
self.free - self.position
}

pub fn take_buffer(&mut self) -> Vec<u8> {
Expand Down Expand Up @@ -160,8 +173,11 @@ impl<RW: AsyncRead + AsyncWrite + Unpin> MinecraftStream<RW> {
where
T: PacketDeserializer,
{
if signature.length > self.data_len() {
match &self.fill_buffer_from_source(signature.length).await {
// signature.length includes the packet_id VarInt which was already consumed
// in read_signature, so subtract its encoded size to get the actual data length
let data_needed = signature.length.saturating_sub(varint_size(signature.packet_id));
if data_needed > self.data_len() {
match &self.fill_buffer_from_source(data_needed).await {
Ok(_) => {}
Err(_) => return Err(ReadingError::Closed),
};
Expand Down Expand Up @@ -199,10 +215,6 @@ impl<RW: AsyncRead + AsyncWrite + Unpin> MinecraftStream<RW> {
T::read(self)
}

fn remain_len(&self) -> usize {
self.buffer.len() - self.position
}

fn copy_buffer_to_start(&mut self) {
let data_len = self.free - self.position;
self.buffer.copy_within(self.position..self.free, 0);
Expand All @@ -211,7 +223,8 @@ impl<RW: AsyncRead + AsyncWrite + Unpin> MinecraftStream<RW> {
}

fn expand_buffer(&mut self) {
todo!()
let new_len = self.buffer.len() * 2;
self.buffer.resize(new_len, 0);
}
Comment on lines 225 to 228
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The buffer expansion logic was changed from todo!() to actual implementation, and the data_len() calculation had an off-by-one error fixed. Given that the minecraft crate has existing test infrastructure, consider adding tests that verify buffer expansion works correctly when the buffer fills up, and that data_len() returns accurate values in various states (empty buffer, partial data, full buffer). These are critical bugs that were fixed and should have regression tests.

Copilot uses AI. Check for mistakes.

async fn fill_buffer_from_source(&mut self, required: usize) -> Result<(), ()> {
Expand Down Expand Up @@ -295,16 +308,15 @@ impl FieldReader for String {
fn read<RW: AsyncRead + AsyncWrite + Unpin>(
stream: &mut MinecraftStream<RW>,
) -> Result<Self, ReadingError> {
// todo: there is a bug - read_field changes position of the stream, but below can happen reading error if packet doesn't fully read
let length = stream.read_field::<i32>()? as usize;

if length > stream.remain_len() {
if length > stream.data_len() {
return Err(ReadingError::Insufficient);
}
let mut vec: Vec<u8> = vec![0; length];
vec.copy_from_slice(&stream.buffer[stream.position..stream.position + length]);
stream.position += length;
Ok(String::from_utf8(vec).unwrap())
String::from_utf8(vec).map_err(|_| ReadingError::Invalid)
}
}

Expand Down
1 change: 1 addition & 0 deletions mineginx/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ tokio = { version = "1.49.0", features = ["full"] }
uuid = { version = "1.20.0", features = ["v4"] }
log = { version = "0.4" }
simple_logger = { version = "5.1.0" }
socket2 = "0.5"
Loading