Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add serde #139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@ keywords = ["ring", "cyclic", "circular", "buffer", "no-std"]
categories = ["data-structures"]
license = "MIT"

[dependencies]
serde = { version = "1.0.217", optional = true, features = ["derive"] }

[dev-dependencies]
criterion = "0.4.0"
compiletest_rs = "0.10.0"
serde_json = "1.0.135"

[features]
default = ["alloc"]
# disable the alloc based ringbuffer, to make RingBuffers work in no_alloc environments
alloc = []
serde = ["alloc", "dep:serde"]

[[bench]]
name = "bench"
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![no_std]
#![deny(missing_docs)]
#![deny(warnings)]
// #![deny(warnings)]
#![deny(unused_import_braces)]
#![deny(unused_results)]
#![deny(trivial_casts)]
Expand Down
111 changes: 111 additions & 0 deletions src/with_alloc/alloc_ringbuffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,107 @@ pub struct AllocRingBuffer<T> {
writeptr: usize,
}

#[cfg(feature = "serde")]
impl<T> serde::Serialize for AllocRingBuffer<T>
where
T: serde::Serialize,
{
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
// Similar to Vec's implementation, we only serialize the actual elements
// Create an iterator over the valid elements
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is an iterator over the valid elements, with self.iter()

let len = if self.writeptr >= self.readptr {
self.writeptr - self.readptr
} else {
self.size - self.readptr + self.writeptr
};

// Use serialize_seq like Vec does
use serde::ser::SerializeSeq;
let mut seq = serializer.serialize_seq(Some(len))?;

// If data is contiguous
if self.writeptr >= self.readptr {
for i in self.readptr..self.writeptr {
// SAFETY: We know the indices are valid and the data is initialized
unsafe {
seq.serialize_element(&*self.buf.add(i))?;
}
}
} else {
// Handle wrapped data - first from readptr to end
for i in self.readptr..self.size {
unsafe {
seq.serialize_element(&*self.buf.add(i))?;
}
}
// Then from start to writeptr
for i in 0..self.writeptr {
unsafe {
seq.serialize_element(&*self.buf.add(i))?;
}
}
}

seq.end()
}
}

#[cfg(feature = "serde")]
impl<'de, T> serde::Deserialize<'de> for AllocRingBuffer<T>
where
T: serde::Deserialize<'de>,
{
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
// Use the same visitor pattern as Vec
struct AllocRingBufferVisitor<T>(core::marker::PhantomData<T>);

impl<'de, T> serde::de::Visitor<'de> for AllocRingBufferVisitor<T>
where
T: serde::Deserialize<'de>,
{
type Value = AllocRingBuffer<T>;

fn expecting(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
formatter.write_str("a sequence")
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: serde::de::SeqAccess<'de>,
{
// Collect elements.
let mut elements = alloc::vec::Vec::new();
while let Some(element) = seq.next_element()? {
elements.push(element);
}

// Now create a properly sized AllocRingBuffer
let mut ringbuffer = AllocRingBuffer::new(elements.len());

// Copy elements into the ringbuffer
unsafe {
ptr::copy_nonoverlapping(elements.as_ptr(), ringbuffer.buf, elements.len());
}

// Update the writeptr to reflect the number of elements
ringbuffer.writeptr = elements.len();
ringbuffer.readptr = 0;

Ok(ringbuffer)
}
}

// Use seq deserializer like Vec does
deserializer.deserialize_seq(AllocRingBufferVisitor(core::marker::PhantomData))
}
}

// SAFETY: all methods that require mutable access take &mut,
// being send and sync was the old behavior but broke when we switched to *mut T.
unsafe impl<T: Sync> Sync for AllocRingBuffer<T> {}
Expand Down Expand Up @@ -474,4 +575,14 @@ mod tests {
assert_eq!(buf.capacity, 4);
assert_eq!(buf.to_vec(), alloc::vec![1, 2, 3, 4]);
}

#[cfg(feature = "serde")]
#[test]
fn serde() {
let a: &[i32] = &[1, 2, 3];
let b = AllocRingBuffer::<i32>::from(a);
let c = serde_json::to_string(&b).unwrap();
let d = serde_json::from_str(&c).unwrap();
assert_eq!(b, d);
}
}
1 change: 1 addition & 0 deletions src/with_alloc/vecdeque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use core::ops::{Deref, DerefMut, Index, IndexMut};
///
/// The reason this is a wrapper, is that we want `RingBuffers` to implement `Index<isize>`,
/// which we cannot do for remote types like `VecDeque`
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct GrowableAllocRingBuffer<T>(VecDeque<T>);

Expand Down
160 changes: 158 additions & 2 deletions src/with_const_generics.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::ringbuffer_trait::{RingBufferIntoIterator, RingBufferIterator, RingBufferMutIterator};
use crate::RingBuffer;
use core::iter::FromIterator;
use core::mem::MaybeUninit;
use core::mem::{self, ManuallyDrop};
use core::mem::{self, ManuallyDrop, MaybeUninit};
use core::ops::{Index, IndexMut};

#[cfg(feature = "serde")]
use serde::{de::MapAccess, ser::SerializeStruct};

/// The `ConstGenericRingBuffer` struct is a `RingBuffer` implementation which does not require `alloc` but
/// uses const generics instead.
///
Expand Down Expand Up @@ -40,6 +42,145 @@ pub struct ConstGenericRingBuffer<T, const CAP: usize> {
writeptr: usize,
}

#[cfg(feature = "serde")]
impl<T, const CAP: usize> serde::Serialize for ConstGenericRingBuffer<T, CAP>
where
T: serde::Serialize,
{
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
// Create a temporary Vec to store the valid elements
let mut elements = alloc::vec::Vec::with_capacity(CAP);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this alloc really necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

since it's basically thrown away


// Handle the case where the buffer might be empty
if self.readptr != self.writeptr {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be better to just use the iterator here

let mut read_idx = self.readptr;

// If writeptr > readptr, elements are contiguous
if self.writeptr > self.readptr {
for idx in self.readptr..self.writeptr {
unsafe {
elements.push(&*self.buf[idx].as_ptr());
}
}
} else {
// Handle wrapped around case
// First read from readptr to end
while read_idx < CAP {
unsafe {
elements.push(&*self.buf[read_idx].as_ptr());
}
read_idx += 1;
}
// Then from start to writeptr
read_idx = 0;
while read_idx < self.writeptr {
unsafe {
elements.push(&*self.buf[read_idx].as_ptr());
}
read_idx += 1;
}
}
}

// Serialize the elements along with the buffer metadata
let mut state = serializer.serialize_struct("ConstGenericRingBuffer", 3)?;
state.serialize_field("elements", &elements)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we need to serialize the read and writeptr tbh. Those aren't useful in a serialization and I'd be very scared deserializing them. Since soundness depends on the state of these pointers, deserializing malformed data could be unsound. Semantically, a serialized ringbufer is just a collection of elements, which when deserialized can just be pushed into a new ringbuffer, which might end up with different states for readptr/writeptr

state.serialize_field("readptr", &self.readptr)?;
state.serialize_field("writeptr", &self.writeptr)?;
state.end()
}
}
#[cfg(feature = "serde")]
impl<'de, T, const CAP: usize> serde::Deserialize<'de> for ConstGenericRingBuffer<T, CAP>
where
T: serde::Deserialize<'de>,
{
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
struct RingBufferVisitor<T, const CAP: usize>(core::marker::PhantomData<T>);

impl<'de, T, const CAP: usize> serde::de::Visitor<'de> for RingBufferVisitor<T, CAP>
where
T: serde::Deserialize<'de>,
{
type Value = ConstGenericRingBuffer<T, CAP>;

fn expecting(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
formatter.write_str("struct ConstGenericRingBuffer")
}

fn visit_map<V>(self, mut map: V) -> Result<Self::Value, V::Error>
where
V: MapAccess<'de>,
{
let mut elements: Option<alloc::vec::Vec<T>> = None;
let mut readptr: Option<usize> = None;
let mut writeptr: Option<usize> = None;

while let Some(key) = map.next_key()? {
match key {
"elements" => {
if elements.is_some() {
return Err(serde::de::Error::duplicate_field("elements"));
}
elements = Some(map.next_value()?);
}
"readptr" => {
if readptr.is_some() {
return Err(serde::de::Error::duplicate_field("readptr"));
}
readptr = Some(map.next_value()?);
}
"writeptr" => {
if writeptr.is_some() {
return Err(serde::de::Error::duplicate_field("writeptr"));
}
writeptr = Some(map.next_value()?);
}
_ => {
return Err(serde::de::Error::unknown_field(
key,
&["elements", "readptr", "writeptr"],
))
}
}
}

let elements =
elements.ok_or_else(|| serde::de::Error::missing_field("elements"))?;
let readptr = readptr.ok_or_else(|| serde::de::Error::missing_field("readptr"))?;
let writeptr =
writeptr.ok_or_else(|| serde::de::Error::missing_field("writeptr"))?;

// Create a new ring buffer with uninitialized memory
let mut buf: [MaybeUninit<T>; CAP] = unsafe { MaybeUninit::uninit().assume_init() };

// Initialize elements in the buffer
for (idx, element) in elements.into_iter().enumerate() {
buf[idx] = MaybeUninit::new(element);
}

Ok(ConstGenericRingBuffer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsound, when data is malformed. if readptr is maliciously (or accidentally) modified it'd read uninitialized elements

buf,
readptr,
writeptr,
})
}
}

deserializer.deserialize_struct(
"ConstGenericRingBuffer",
&["elements", "readptr", "writeptr"],
RingBufferVisitor(core::marker::PhantomData),
)
}
}

impl<T, const CAP: usize> From<[T; CAP]> for ConstGenericRingBuffer<T, CAP> {
fn from(value: [T; CAP]) -> Self {
let v = ManuallyDrop::new(value);
Expand Down Expand Up @@ -496,4 +637,19 @@ mod tests {
vec![1, 2, 3]
);
}

#[cfg(feature = "serde")]
#[test]
fn serde() {
let a: &[i32] = &[];
let b = ConstGenericRingBuffer::<i32, 3>::from(a);
let c = serde_json::to_string(&b).unwrap();
let d = serde_json::from_str(&c).unwrap();
assert_eq!(b, d);
let a: &[i32] = &[1, 2, 3];
let b = ConstGenericRingBuffer::<i32, 3>::from(a);
let c = serde_json::to_string(&b).unwrap();
let d = serde_json::from_str(&c).unwrap();
assert_eq!(b, d);
}
}