From d13483233f7b5094b6c20238c4bf4432b1e7a50d Mon Sep 17 00:00:00 2001 From: Ryan Johnson Date: Tue, 15 Aug 2023 08:14:47 -0700 Subject: [PATCH] Consolidate unsafe byte conversions in valence_nbt (#465) # Objective When using valence_nbt, it's often necessary to convert to and from slices and vecs of `i8` and `u8`. But this requires unsafe code if you want to avoid copying things. # Solution Expose the following functions in valence_nbt: - `u8_vec_into_i8_vec(vec: Vec) -> Vec` - `i8_vec_into_u8_vec(vec: Vec) -> Vec` - `u8_slice_as_i8_slice(slice: &[u8]) -> &[i8]` - `i8_slice_as_u8_slice(slice: &[i8]) -> &[u8]` We've also made use of these functions ourselves to reduce the total amount of unsafe code. Should also help in #263 --- crates/valence_nbt/src/binary/decode.rs | 13 ++------- crates/valence_nbt/src/lib.rs | 38 +++++++++++++++++++++++++ crates/valence_nbt/src/serde.rs | 21 -------------- crates/valence_nbt/src/serde/de.rs | 23 ++++++--------- crates/valence_nbt/src/serde/ser.rs | 14 +++------ crates/valence_protocol/src/impls.rs | 11 ++----- 6 files changed, 56 insertions(+), 64 deletions(-) diff --git a/crates/valence_nbt/src/binary/decode.rs b/crates/valence_nbt/src/binary/decode.rs index 86598aaa0..5788f3e24 100644 --- a/crates/valence_nbt/src/binary/decode.rs +++ b/crates/valence_nbt/src/binary/decode.rs @@ -1,11 +1,10 @@ use std::io::Write; -use std::slice; use byteorder::{BigEndian, WriteBytesExt}; use super::{modified_utf8, Error, Result}; use crate::tag::Tag; -use crate::{Compound, List, Value}; +use crate::{i8_slice_as_u8_slice, Compound, List, Value}; impl Compound { /// Encodes uncompressed NBT binary data to the provided writer. @@ -146,10 +145,7 @@ impl EncodeState { } } - // SAFETY: i8 has the same layout as u8. - let bytes = unsafe { slice::from_raw_parts(bytes.as_ptr() as *const u8, bytes.len()) }; - - Ok(self.writer.write_all(bytes)?) + Ok(self.writer.write_all(i8_slice_as_u8_slice(bytes))?) } fn write_string(&mut self, s: &str) -> Result<()> { @@ -197,10 +193,7 @@ impl EncodeState { } } - // SAFETY: i8 has the same layout as u8. - let bytes = unsafe { slice::from_raw_parts(bl.as_ptr() as *const u8, bl.len()) }; - - Ok(self.writer.write_all(bytes)?) + Ok(self.writer.write_all(i8_slice_as_u8_slice(bl))?) } List::Short(sl) => self.write_list(sl, Tag::Short, |st, s| st.write_short(*s)), List::Int(il) => self.write_list(il, Tag::Int, |st, i| st.write_int(*i)), diff --git a/crates/valence_nbt/src/lib.rs b/crates/valence_nbt/src/lib.rs index addcee0fe..4d35ce2b7 100644 --- a/crates/valence_nbt/src/lib.rs +++ b/crates/valence_nbt/src/lib.rs @@ -17,6 +17,8 @@ clippy::dbg_macro )] +use std::mem::ManuallyDrop; + pub use compound::Compound; pub use tag::Tag; pub use value::{List, Value}; @@ -75,3 +77,39 @@ macro_rules! compound { ]) } } + +/// Converts a `Vec` into a `Vec` without cloning. +#[inline] +pub fn u8_vec_into_i8_vec(vec: Vec) -> Vec { + // SAFETY: Layouts of u8 and i8 are the same and we're being careful not to drop + // the original vec after calling Vec::from_raw_parts. + unsafe { + let mut vec = ManuallyDrop::new(vec); + Vec::from_raw_parts(vec.as_mut_ptr() as *mut i8, vec.len(), vec.capacity()) + } +} + +/// Converts a `Vec` into a `Vec` without cloning. +#[inline] +pub fn i8_vec_into_u8_vec(vec: Vec) -> Vec { + // SAFETY: Layouts of u8 and i8 are the same and we're being careful not to drop + // the original vec after calling Vec::from_raw_parts. + unsafe { + let mut vec = ManuallyDrop::new(vec); + Vec::from_raw_parts(vec.as_mut_ptr() as *mut u8, vec.len(), vec.capacity()) + } +} + +/// Converts a `&[u8]` into a `&[i8]`. +#[inline] +pub fn u8_slice_as_i8_slice(slice: &[u8]) -> &[i8] { + // SAFETY: i8 has the same layout as u8. + unsafe { std::slice::from_raw_parts(slice.as_ptr() as *const i8, slice.len()) } +} + +/// Converts a `&[i8]` into a `&[u8]`. +#[inline] +pub fn i8_slice_as_u8_slice(slice: &[i8]) -> &[u8] { + // SAFETY: i8 has the same layout as u8. + unsafe { std::slice::from_raw_parts(slice.as_ptr() as *const u8, slice.len()) } +} diff --git a/crates/valence_nbt/src/serde.rs b/crates/valence_nbt/src/serde.rs index 7574b2bb4..8159f5d7d 100644 --- a/crates/valence_nbt/src/serde.rs +++ b/crates/valence_nbt/src/serde.rs @@ -1,5 +1,4 @@ use std::fmt; -use std::mem::ManuallyDrop; pub use ser::*; use thiserror::Error; @@ -38,23 +37,3 @@ impl serde::ser::Error for Error { Self::new(format!("{msg}")) } } - -#[inline] -fn u8_vec_to_i8_vec(vec: Vec) -> Vec { - // SAFETY: Layouts of u8 and i8 are the same and we're being careful not to drop - // the original vec after calling Vec::from_raw_parts. - unsafe { - let mut vec = ManuallyDrop::new(vec); - Vec::from_raw_parts(vec.as_mut_ptr() as *mut i8, vec.len(), vec.capacity()) - } -} - -#[inline] -fn i8_vec_to_u8_vec(vec: Vec) -> Vec { - // SAFETY: Layouts of u8 and i8 are the same and we're being careful not to drop - // the original vec after calling Vec::from_raw_parts. - unsafe { - let mut vec = ManuallyDrop::new(vec); - Vec::from_raw_parts(vec.as_mut_ptr() as *mut u8, vec.len(), vec.capacity()) - } -} diff --git a/crates/valence_nbt/src/serde/de.rs b/crates/valence_nbt/src/serde/de.rs index da0178243..9bc5f7327 100644 --- a/crates/valence_nbt/src/serde/de.rs +++ b/crates/valence_nbt/src/serde/de.rs @@ -1,12 +1,11 @@ -use std::{fmt, slice}; +use std::fmt; use serde::de::value::{MapAccessDeserializer, MapDeserializer, SeqAccessDeserializer}; use serde::de::{self, IntoDeserializer, SeqAccess, Visitor}; use serde::{forward_to_deserialize_any, Deserialize, Deserializer}; use super::Error; -use crate::serde::{i8_vec_to_u8_vec, u8_vec_to_i8_vec}; -use crate::{Compound, List, Value}; +use crate::{i8_vec_into_u8_vec, u8_slice_as_i8_slice, u8_vec_into_i8_vec, Compound, List, Value}; impl<'de> Deserialize<'de> for Value { fn deserialize(deserializer: D) -> Result @@ -117,17 +116,14 @@ impl<'de> Deserialize<'de> for Value { where E: de::Error, { - let slice: &[i8] = - unsafe { slice::from_raw_parts(v.as_ptr() as *const i8, v.len()) }; - - Ok(Value::ByteArray(slice.into())) + Ok(Value::ByteArray(u8_slice_as_i8_slice(v).into())) } fn visit_byte_buf(self, v: Vec) -> Result where E: de::Error, { - Ok(Value::ByteArray(u8_vec_to_i8_vec(v))) + Ok(Value::ByteArray(u8_vec_into_i8_vec(v))) } fn visit_seq(self, seq: A) -> Result @@ -190,17 +186,14 @@ impl<'de> Deserialize<'de> for List { where E: de::Error, { - Ok(List::Byte(u8_vec_to_i8_vec(v))) + Ok(List::Byte(u8_vec_into_i8_vec(v))) } fn visit_bytes(self, v: &[u8]) -> Result where E: de::Error, { - let bytes: &[i8] = - unsafe { slice::from_raw_parts(v.as_ptr() as *const i8, v.len()) }; - - Ok(List::Byte(bytes.into())) + Ok(List::Byte(u8_slice_as_i8_slice(v).into())) } } @@ -269,7 +262,7 @@ impl<'de> Deserializer<'de> for Value { Value::Long(v) => visitor.visit_i64(v), Value::Float(v) => visitor.visit_f32(v), Value::Double(v) => visitor.visit_f64(v), - Value::ByteArray(v) => visitor.visit_byte_buf(i8_vec_to_u8_vec(v)), + Value::ByteArray(v) => visitor.visit_byte_buf(i8_vec_into_u8_vec(v)), Value::String(v) => visitor.visit_string(v), Value::List(v) => v.deserialize_any(visitor), Value::Compound(v) => v.into_deserializer().deserialize_any(visitor), @@ -347,7 +340,7 @@ impl<'de> Deserializer<'de> for List { match self { List::End => visitor.visit_seq(EndSeqAccess), - List::Byte(v) => visitor.visit_byte_buf(i8_vec_to_u8_vec(v)), + List::Byte(v) => visitor.visit_byte_buf(i8_vec_into_u8_vec(v)), List::Short(v) => v.into_deserializer().deserialize_any(visitor), List::Int(v) => v.into_deserializer().deserialize_any(visitor), List::Long(v) => v.into_deserializer().deserialize_any(visitor), diff --git a/crates/valence_nbt/src/serde/ser.rs b/crates/valence_nbt/src/serde/ser.rs index 29993cecc..4b76e0131 100644 --- a/crates/valence_nbt/src/serde/ser.rs +++ b/crates/valence_nbt/src/serde/ser.rs @@ -1,11 +1,10 @@ -use core::slice; use std::marker::PhantomData; use serde::ser::{Impossible, SerializeMap, SerializeSeq, SerializeStruct}; use serde::{Serialize, Serializer}; -use super::{u8_vec_to_i8_vec, Error}; -use crate::{Compound, List, Value}; +use super::Error; +use crate::{i8_slice_as_u8_slice, u8_vec_into_i8_vec, Compound, List, Value}; impl Serialize for Value { fn serialize(&self, serializer: S) -> Result @@ -19,12 +18,7 @@ impl Serialize for Value { Value::Long(v) => serializer.serialize_i64(*v), Value::Float(v) => serializer.serialize_f32(*v), Value::Double(v) => serializer.serialize_f64(*v), - Value::ByteArray(v) => { - // SAFETY: i8 has the same layout as u8. - let bytes = unsafe { slice::from_raw_parts(v.as_ptr() as *const u8, v.len()) }; - - serializer.serialize_bytes(bytes) - } + Value::ByteArray(v) => serializer.serialize_bytes(i8_slice_as_u8_slice(v)), Value::String(v) => serializer.serialize_str(v), Value::List(v) => v.serialize(serializer), Value::Compound(v) => v.serialize(serializer), @@ -317,7 +311,7 @@ impl Serializer for ValueSerializer { } fn serialize_bytes(self, v: &[u8]) -> Result { - Ok(Value::ByteArray(u8_vec_to_i8_vec(v.into()))) + Ok(Value::ByteArray(u8_vec_into_i8_vec(v.into()))) } fn serialize_none(self) -> Result { diff --git a/crates/valence_protocol/src/impls.rs b/crates/valence_protocol/src/impls.rs index 857634693..f95f71d11 100644 --- a/crates/valence_protocol/src/impls.rs +++ b/crates/valence_protocol/src/impls.rs @@ -18,7 +18,7 @@ use valence_generated::block::{BlockEntityKind, BlockKind, BlockState}; use valence_generated::item::ItemKind; use valence_ident::{Ident, IdentError}; use valence_math::*; -use valence_nbt::Compound; +use valence_nbt::{i8_slice_as_u8_slice, u8_slice_as_i8_slice, Compound}; use valence_text::Text; use super::var_int::VarInt; @@ -69,9 +69,7 @@ impl Encode for i8 { } fn encode_slice(slice: &[i8], mut w: impl Write) -> Result<()> { - // SAFETY: i8 has the same layout as u8. - let bytes = unsafe { slice::from_raw_parts(slice.as_ptr() as *const u8, slice.len()) }; - Ok(w.write_all(bytes)?) + Ok(w.write_all(i8_slice_as_u8_slice(slice))?) } } @@ -546,10 +544,7 @@ impl<'a> Decode<'a> for &'a [i8] { fn decode(r: &mut &'a [u8]) -> Result { let bytes = <&[u8]>::decode(r)?; - // SAFETY: i8 and u8 have the same layout. - let bytes = unsafe { slice::from_raw_parts(bytes.as_ptr() as *const i8, bytes.len()) }; - - Ok(bytes) + Ok(u8_slice_as_i8_slice(bytes)) } }