From 4f4c3b172dbc42596b5b0423de9b0e09dc8f7535 Mon Sep 17 00:00:00 2001 From: Kaydax Date: Thu, 13 Nov 2025 21:57:47 -0500 Subject: [PATCH 1/2] Attempt to fix crash when protocol is negative --- minecraft/src/serialization.rs | 122 +++++++++++++++++---------- minecraft/src/tests/field_types.rs | 28 ++++++ minecraft/src/tests/serialization.rs | 42 +++++++-- 3 files changed, 141 insertions(+), 51 deletions(-) diff --git a/minecraft/src/serialization.rs b/minecraft/src/serialization.rs index 4d5dc6e..009cfe8 100644 --- a/minecraft/src/serialization.rs +++ b/minecraft/src/serialization.rs @@ -1,36 +1,38 @@ - use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; use uuid::Uuid; -use crate::{buffer::Buffer, packets::{MinecraftPacket, PacketDeserializer, PacketSerializer}}; +use crate::{ + buffer::Buffer, + packets::{MinecraftPacket, PacketDeserializer, PacketSerializer}, +}; const SEGMENT_BITS: i32 = 0x7F; const CONTINUE_BIT: i32 = 0x80; -#[derive(Debug)] -#[derive(PartialEq)] +#[derive(Debug, PartialEq)] pub enum ReadingError { Insufficient, Invalid, - Closed + Closed, } impl From for () { fn from(value: ReadingError) -> Self { let _ = value; - } } -#[derive(Debug)] -#[derive(PartialEq)] +#[derive(Debug, PartialEq)] pub struct Signature { pub length: usize, - pub packet_id: i32 + pub packet_id: i32, } impl FieldWriter for Signature { - fn write(&self, stream: &mut Buffer) -> Option<()> where Self: Sized { + fn write(&self, stream: &mut Buffer) -> Option<()> + where + Self: Sized, + { (self.length as i32).write(stream); 0.write(stream); Some(()) @@ -45,11 +47,16 @@ pub(crate) trait FieldReader { } pub(crate) trait FieldWriter { - fn write(&self, stream: &mut Buffer) -> Option<()> where Self: Sized; + fn write(&self, stream: &mut Buffer) -> Option<()> + where + Self: Sized; } impl Buffer { - pub(crate) fn write_field(&mut self, value: &T) -> Option<()> where T: FieldWriter { + pub(crate) fn write_field(&mut self, value: &T) -> Option<()> + where + T: FieldWriter, + { T::write(value, self) } } @@ -61,7 +68,10 @@ impl Buffer { /// `position` points to the start of used memory /// `free` points to the start of not used yet /// if there is no space left in not used yet memory, used memory will copy to the start of buffer -pub struct MinecraftStream where RW : AsyncRead + AsyncWrite + Unpin { +pub struct MinecraftStream +where + RW: AsyncRead + AsyncWrite + Unpin, +{ buffer: Vec, client: RW, free: usize, @@ -74,7 +84,7 @@ impl MinecraftStream { buffer: vec![0; init_buffer_size], client, position: 0, - free: 0 + free: 0, } } @@ -103,10 +113,9 @@ impl MinecraftStream { Err(e) => { if e == ReadingError::Invalid { return Err(e); - } - else if e == ReadingError::Insufficient { + } else if e == ReadingError::Insufficient { match self.fill_buffer_from_source(0).await { - Ok(_) => { }, + Ok(_) => {} Err(_) => return Err(ReadingError::Closed), } continue; @@ -122,10 +131,9 @@ impl MinecraftStream { Err(e) => { if e == ReadingError::Invalid { return Err(e); - } - else if e == ReadingError::Insufficient { + } else if e == ReadingError::Insufficient { match self.fill_buffer_from_source(0).await { - Ok(_) => { }, + Ok(_) => {} Err(_) => return Err(ReadingError::Closed), } continue; @@ -142,17 +150,20 @@ impl MinecraftStream { Ok(Signature { packet_id, - length: length as usize + length: length as usize, }) } /// Reads `data` field of packet to the end /// https://wiki.vg/Protocol#Packet_format - pub async fn read_data(&mut self, signature: Signature) -> Result where T : PacketDeserializer { + pub async fn read_data(&mut self, signature: Signature) -> Result + where + T: PacketDeserializer, + { if signature.length > self.data_len() { match &self.fill_buffer_from_source(signature.length).await { - Ok(_) => {}, - Err(_) => return Err(ReadingError::Closed) + Ok(_) => {} + Err(_) => return Err(ReadingError::Closed), }; } @@ -161,21 +172,30 @@ impl MinecraftStream { /// Reads **exactly this packet** to the end ignoring packet id from signature. /// Return error if client close the connection - pub async fn read_packet(&mut self) -> Result where T: PacketDeserializer { + pub async fn read_packet(&mut self) -> Result + where + T: PacketDeserializer, + { let signature = self.read_signature().await?; self.read_data(signature).await } - pub async fn write_packet(&mut self, packet: &T) -> Option<()> where T: PacketSerializer { + pub async fn write_packet(&mut self, packet: &T) -> Option<()> + where + T: PacketSerializer, + { let packet = MinecraftPacket::make_raw(0, packet)?; match self.client.write_all(&packet[0..packet.len()]).await { - Ok(_) => { }, + Ok(_) => {} Err(_) => return None, }; Some(()) } - pub(crate) fn read_field(&mut self) -> Result where T: FieldReader { + pub(crate) fn read_field(&mut self) -> Result + where + T: FieldReader, + { T::read(self) } @@ -198,8 +218,7 @@ impl MinecraftStream { if self.free >= self.buffer.len() { if self.position != 0 { self.copy_buffer_to_start(); - } - else { + } else { self.expand_buffer(); } } @@ -212,7 +231,7 @@ impl MinecraftStream { return Err(()); } self.free += size; - }, + } Err(_) => { return Err(()); } @@ -228,7 +247,9 @@ impl MinecraftStream { } impl FieldReader for i32 { - fn read(stream: &mut MinecraftStream) -> Result { + fn read( + stream: &mut MinecraftStream, + ) -> Result { let mut value = 0; let mut current_position = 0; let mut current_byte: i32; @@ -258,20 +279,22 @@ impl FieldReader for i32 { impl FieldWriter for i32 { fn write(&self, stream: &mut Buffer) -> Option<()> { - let mut value = *self; + let mut value = *self as u32; loop { - if (value & !SEGMENT_BITS) == 0 { + if (value & !(SEGMENT_BITS as u32)) == 0 { stream.write_byte(value as u8); - return Some(()) + return Some(()); } - stream.write_byte(((value & SEGMENT_BITS) | CONTINUE_BIT) as u8); + stream.write_byte(((value & SEGMENT_BITS as u32) | CONTINUE_BIT as u32) as u8); value >>= 7; } } } impl FieldReader for String { - fn read(stream: &mut MinecraftStream) -> Result { + fn read( + stream: &mut MinecraftStream, + ) -> Result { // 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::()? as usize; @@ -297,7 +320,9 @@ impl FieldWriter for String { } impl FieldReader for u16 { - fn read(stream: &mut MinecraftStream) -> Result { + fn read( + stream: &mut MinecraftStream, + ) -> Result { if stream.data_len() < 2 { return Err(ReadingError::Insufficient); } @@ -323,7 +348,9 @@ impl FieldWriter for u16 { } impl FieldReader for bool { - fn read(stream: &mut MinecraftStream) -> Result { + fn read( + stream: &mut MinecraftStream, + ) -> Result { let byte = stream.read_field::()?; Ok(byte != 0) } @@ -337,7 +364,12 @@ impl FieldWriter for bool { } impl FieldReader for u8 { - fn read(stream: &mut MinecraftStream) -> Result where Self : Sized { + fn read( + stream: &mut MinecraftStream, + ) -> Result + where + Self: Sized, + { if stream.position >= stream.free { return Err(ReadingError::Insufficient); } @@ -348,7 +380,9 @@ impl FieldReader for u8 { } impl FieldReader for Uuid { - fn read(stream: &mut MinecraftStream) -> Result { + fn read( + stream: &mut MinecraftStream, + ) -> Result { if stream.data_len() < 16 { return Err(ReadingError::Insufficient); } @@ -357,8 +391,8 @@ impl FieldReader for Uuid { Ok(v) => { stream.position += 16; Ok(v) - }, - Err(_) => Err(ReadingError::Insufficient) + } + Err(_) => Err(ReadingError::Insufficient), } } } @@ -376,6 +410,6 @@ pub fn truncate_to_zero(value: &str) -> &str { let index = &value.find('\0'); match index { Some(v) => &value[0..*v], - None => value + None => value, } } diff --git a/minecraft/src/tests/field_types.rs b/minecraft/src/tests/field_types.rs index 88b2979..53e211e 100644 --- a/minecraft/src/tests/field_types.rs +++ b/minecraft/src/tests/field_types.rs @@ -14,4 +14,32 @@ fn bool_write_false() { assert_eq!(buffer.take()[0], 0); } +#[test] +fn i32_write_positive() { + let mut buffer = Buffer::new(1024); + 300.write(&mut buffer); + let bytes = buffer.take(); + // 300 in VarInt: 0xAC 0x02 + assert_eq!(bytes, &[0xAC, 0x02]); +} + +#[test] +fn i32_write_negative_one() { + let mut buffer = Buffer::new(1024); + (-1_i32).write(&mut buffer); + let bytes = buffer.take(); + // -1 in VarInt should be encoded properly (5 bytes for negative numbers) + // -1 = 0xFFFFFFFF in binary, encoded as VarInt + assert_eq!(bytes.len(), 5); + assert_eq!(bytes, &[0xFF, 0xFF, 0xFF, 0xFF, 0x0F]); +} + +#[test] +fn i32_write_zero() { + let mut buffer = Buffer::new(1024); + 0.write(&mut buffer); + let bytes = buffer.take(); + assert_eq!(bytes, &[0x00]); +} + // todo: make more tests for FieldWriter and FieldReader diff --git a/minecraft/src/tests/serialization.rs b/minecraft/src/tests/serialization.rs index f82a4ec..8409d4f 100644 --- a/minecraft/src/tests/serialization.rs +++ b/minecraft/src/tests/serialization.rs @@ -39,12 +39,14 @@ async fn write_packet() { let mut stream = BufStream::new(Cursor::new(vec![0; 1024])); { let mut minecraft = MinecraftStream::new(stream.borrow_mut(), 1024); - minecraft.write_packet(&HandshakeC2SPacket { - protocol_version: 16, - domain: "net".to_owned(), - server_port: 65535, - next_state: 2 - }).await; + minecraft + .write_packet(&HandshakeC2SPacket { + protocol_version: 16, + domain: "net".to_owned(), + server_port: 65535, + next_state: 2, + }) + .await; } let mut array = vec![0_u8; 1024]; stream.seek(std::io::SeekFrom::Start(0)).await.unwrap(); @@ -58,8 +60,34 @@ async fn write_packet() { assert_eq!(packet.next_state, 2); } +#[tokio::test] +async fn write_packet_with_negative_protocol_version() { + let mut stream = BufStream::new(Cursor::new(vec![0; 1024])); + { + let mut minecraft = MinecraftStream::new(stream.borrow_mut(), 1024); + minecraft + .write_packet(&HandshakeC2SPacket { + protocol_version: -1, + domain: "mc.kaydax.xyz".to_owned(), + server_port: 25565, + next_state: 1, + }) + .await; + } + let mut array = vec![0_u8; 1024]; + stream.seek(std::io::SeekFrom::Start(0)).await.unwrap(); + _ = stream.read(&mut array[0..1024]).await.unwrap(); + stream.seek(std::io::SeekFrom::Start(0)).await.unwrap(); + let mut minecraft = MinecraftStream::new(stream.borrow_mut(), 1024); + let packet = minecraft.read_packet::().await.unwrap(); + assert_eq!(packet.protocol_version, -1); + assert_eq!(packet.domain, "mc.kaydax.xyz"); + assert_eq!(packet.server_port, 25565); + assert_eq!(packet.next_state, 1); +} + fn make_minecraft_stream(array: Vec) -> MinecraftStream>>> { let stream = BufStream::new(Cursor::new(array.clone())); - + MinecraftStream::new(stream, 1024) } From 8d7c4568f5d5e9ea0040ac603ee08dc62c11b6e8 Mon Sep 17 00:00:00 2001 From: Folleach Date: Sat, 22 Nov 2025 12:25:26 +0500 Subject: [PATCH 2/2] replace current value with unsigned when reading --- minecraft/src/serialization.rs | 10 +++++----- minecraft/src/tests/serialization.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/minecraft/src/serialization.rs b/minecraft/src/serialization.rs index 009cfe8..34e16e0 100644 --- a/minecraft/src/serialization.rs +++ b/minecraft/src/serialization.rs @@ -6,8 +6,8 @@ use crate::{ packets::{MinecraftPacket, PacketDeserializer, PacketSerializer}, }; -const SEGMENT_BITS: i32 = 0x7F; -const CONTINUE_BIT: i32 = 0x80; +const SEGMENT_BITS: u32 = 0x7F; +const CONTINUE_BIT: u32 = 0x80; #[derive(Debug, PartialEq)] pub enum ReadingError { @@ -252,14 +252,14 @@ impl FieldReader for i32 { ) -> Result { let mut value = 0; let mut current_position = 0; - let mut current_byte: i32; + let mut current_byte: u32; let mut index = stream.position; loop { if index >= stream.free { return Err(ReadingError::Insufficient); } - current_byte = stream.buffer[index] as i32; + current_byte = stream.buffer[index] as u32; index += 1; value |= (current_byte & SEGMENT_BITS) << current_position; @@ -273,7 +273,7 @@ impl FieldReader for i32 { } stream.position = index; - Ok(value) + Ok(value as i32) } } diff --git a/minecraft/src/tests/serialization.rs b/minecraft/src/tests/serialization.rs index 8409d4f..cd761d4 100644 --- a/minecraft/src/tests/serialization.rs +++ b/minecraft/src/tests/serialization.rs @@ -86,6 +86,32 @@ async fn write_packet_with_negative_protocol_version() { assert_eq!(packet.next_state, 1); } +#[tokio::test] +async fn i32_write_and_read_large_negative() { + let mut stream = BufStream::new(Cursor::new(vec![0; 1024])); + { + let mut minecraft = MinecraftStream::new(stream.borrow_mut(), 1024); + minecraft + .write_packet(&HandshakeC2SPacket { + protocol_version: 1, + domain: "mc.kaydax.xyz".to_owned(), + server_port: 25565, + next_state: -1599979007, + }) + .await; + } + let mut array = vec![0_u8; 1024]; + stream.seek(std::io::SeekFrom::Start(0)).await.unwrap(); + _ = stream.read(&mut array[0..1024]).await.unwrap(); + stream.seek(std::io::SeekFrom::Start(0)).await.unwrap(); + let mut minecraft = MinecraftStream::new(stream.borrow_mut(), 1024); + let packet = minecraft.read_packet::().await.unwrap(); + assert_eq!(packet.protocol_version, 1); + assert_eq!(packet.domain, "mc.kaydax.xyz"); + assert_eq!(packet.server_port, 25565); + assert_eq!(packet.next_state, -1599979007); +} + fn make_minecraft_stream(array: Vec) -> MinecraftStream>>> { let stream = BufStream::new(Cursor::new(array.clone()));