Improve overall `unsafe` hygiene
This adds `#![deny(unsafe_op_in_unsafe_fn)]` which removes the
implicit `unsafe` block that `unsafe fn` does.
It also adds many more `SAFETY` docs, corrects some incomplete
ones, and catches a null pointer returned by `upb_Arena_New`.
PiperOrigin-RevId: 549067106
diff --git a/rust/cpp.rs b/rust/cpp.rs
index b60ef58..deac9d9 100644
--- a/rust/cpp.rs
+++ b/rust/cpp.rs
@@ -30,15 +30,13 @@
// Rust Protobuf runtime using the C++ kernel.
-use std::alloc;
use std::alloc::Layout;
use std::cell::UnsafeCell;
use std::fmt;
use std::marker::PhantomData;
use std::mem::MaybeUninit;
use std::ops::Deref;
-use std::ptr::NonNull;
-use std::slice;
+use std::ptr::{self, NonNull};
/// A wrapper over a `proto2::Arena`.
///
@@ -72,7 +70,7 @@
///
/// # Safety
///
- /// `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`.
+ /// TODO alignment requirement for layout
#[inline]
pub unsafe fn alloc(&self, _layout: Layout) -> &mut [MaybeUninit<u8>] {
unimplemented!()
@@ -83,8 +81,8 @@
/// # Safety
///
/// After calling this function, `ptr` is essentially zapped. `old` must
- /// be the layout `ptr` was allocated with via [`Arena::alloc()`]. `new`'s
- /// alignment must be less than `UPB_MALLOC_ALIGN`.
+ /// be the layout `ptr` was allocated with via [`Arena::alloc()`].
+ /// TODO alignment for layout
#[inline]
pub unsafe fn resize(&self, _ptr: *mut u8, _old: Layout, _new: Layout) -> &[MaybeUninit<u8>] {
unimplemented!()
@@ -111,23 +109,42 @@
}
impl SerializedData {
+ /// Constructs owned serialized data from raw components.
+ ///
+ /// # Safety
+ /// - `data` must be readable for `len` bytes.
+ /// - `data` must be an owned pointer and valid until deallocated.
+ /// - `data` must have been allocated by the Rust global allocator with a
+ /// size of `len` and align of 1.
pub unsafe fn from_raw_parts(data: NonNull<u8>, len: usize) -> Self {
Self { data, len }
}
+
+ /// Gets a raw slice pointer.
+ pub fn as_ptr(&self) -> *const [u8] {
+ ptr::slice_from_raw_parts(self.data.as_ptr(), self.len)
+ }
+
+ /// Gets a mutable raw slice pointer.
+ fn as_mut_ptr(&mut self) -> *mut [u8] {
+ ptr::slice_from_raw_parts_mut(self.data.as_ptr(), self.len)
+ }
}
impl Deref for SerializedData {
type Target = [u8];
fn deref(&self) -> &Self::Target {
- unsafe { slice::from_raw_parts(self.data.as_ptr(), self.len) }
+ // SAFETY: `data` is valid for `len` bytes until deallocated as promised by
+ // `from_raw_parts`.
+ unsafe { &*self.as_ptr() }
}
}
impl Drop for SerializedData {
fn drop(&mut self) {
- unsafe {
- alloc::dealloc(self.data.as_ptr(), Layout::array::<u8>(self.len).unwrap());
- };
+ // SAFETY: `data` was allocated by the Rust global allocator with a
+ // size of `len` and align of 1 as promised by `from_raw_parts`.
+ unsafe { drop(Box::from_raw(self.as_mut_ptr())) }
}
}
diff --git a/rust/cpp_kernel/cpp_api.h b/rust/cpp_kernel/cpp_api.h
index 78357a1..a9e18fc 100644
--- a/rust/cpp_kernel/cpp_api.h
+++ b/rust/cpp_kernel/cpp_api.h
@@ -50,7 +50,7 @@
// * The data were allocated using the Rust allocator.
//
extern "C" struct SerializedData {
- /// Owns the memory.
+ // Owns the memory.
const char* data;
size_t len;
diff --git a/rust/internal.rs b/rust/internal.rs
index ec4fd00..ada1025 100644
--- a/rust/internal.rs
+++ b/rust/internal.rs
@@ -61,14 +61,17 @@
/// Unsafely dereference this slice.
///
/// # Safety
- /// - `ptr` must be valid for `len` bytes. It can be null or dangling if
- /// `self.len == 0`.
+ /// - `self.ptr` must be dereferencable and immutable for `self.len` bytes
+ /// for the lifetime `'a`. It can be null or dangling if `self.len == 0`.
pub unsafe fn as_ref<'a>(self) -> &'a [u8] {
if self.ptr.is_null() {
assert_eq!(self.len, 0, "Non-empty slice with null data pointer");
&[]
} else {
- slice::from_raw_parts(self.ptr, self.len)
+ // SAFETY:
+ // - `ptr` is non-null
+ // - `ptr` is valid for `len` bytes as promised by the caller.
+ unsafe { slice::from_raw_parts(self.ptr, self.len) }
}
}
}
diff --git a/rust/shared.rs b/rust/shared.rs
index f27b5bc..0e8b7d2 100644
--- a/rust/shared.rs
+++ b/rust/shared.rs
@@ -32,6 +32,19 @@
//!
//! For kernel-specific logic this crate delegates to the respective `__runtime`
//! crate.
+#![deny(unsafe_op_in_unsafe_fn)]
+
+use std::fmt;
+
+pub use optional::{AbsentField, FieldEntry, Optional, PresentField};
+pub use proxied::{Mut, MutProxy, Proxied, ProxiedWithPresence, SettableValue, View, ViewProxy};
+pub use string::{BytesMut, ProtoStr};
+
+/// Everything in `__internal` is allowed to change without it being considered
+/// a breaking change for the protobuf library. Nothing in here should be
+/// exported in `protobuf.rs`.
+#[path = "internal.rs"]
+pub mod __internal;
/// Everything in `__runtime` is allowed to change without it being considered
/// a breaking change for the protobuf library. Nothing in here should be
@@ -47,18 +60,6 @@
mod proxied;
mod string;
-pub use optional::{AbsentField, FieldEntry, Optional, PresentField};
-pub use proxied::{Mut, MutProxy, Proxied, ProxiedWithPresence, SettableValue, View, ViewProxy};
-pub use string::{BytesMut, ProtoStr};
-
-/// Everything in `__internal` is allowed to change without it being considered
-/// a breaking change for the protobuf library. Nothing in here should be
-/// exported in `protobuf.rs`.
-#[path = "internal.rs"]
-pub mod __internal;
-
-use std::fmt;
-
/// An error that happened during deserialization.
#[derive(Debug, Clone)]
pub struct ParseError;
diff --git a/rust/string.rs b/rust/string.rs
index 88464bf..fe9d827 100644
--- a/rust/string.rs
+++ b/rust/string.rs
@@ -31,7 +31,6 @@
//! Items specific to `bytes` and `string` fields.
#![allow(dead_code)]
#![allow(unused)]
-#![deny(unsafe_op_in_unsafe_fn)]
use crate::__internal::Private;
use crate::{Mut, MutProxy, Proxied, ProxiedWithPresence, SettableValue, View, ViewProxy};
diff --git a/rust/upb.rs b/rust/upb.rs
index 11c2246..e1e795a 100644
--- a/rust/upb.rs
+++ b/rust/upb.rs
@@ -37,7 +37,7 @@
use std::marker::PhantomData;
use std::mem::MaybeUninit;
use std::ops::Deref;
-use std::ptr::NonNull;
+use std::ptr::{self, NonNull};
use std::slice;
/// See `upb/port/def.inc`.
@@ -62,12 +62,14 @@
///
/// Note that this type is neither `Sync` nor `Send`.
pub struct Arena {
+ // Safety invariant: this must always be a valid arena
raw: RawArena,
_not_sync: PhantomData<UnsafeCell<()>>,
}
extern "C" {
- fn upb_Arena_New() -> RawArena;
+ // `Option<NonNull<T: Sized>>` is ABI-compatible with `*mut T`
+ fn upb_Arena_New() -> Option<RawArena>;
fn upb_Arena_Free(arena: RawArena);
fn upb_Arena_Malloc(arena: RawArena, size: usize) -> *mut u8;
fn upb_Arena_Realloc(arena: RawArena, ptr: *mut u8, old: usize, new: usize) -> *mut u8;
@@ -77,7 +79,19 @@
/// Allocates a fresh arena.
#[inline]
pub fn new() -> Self {
- Self { raw: unsafe { upb_Arena_New() }, _not_sync: PhantomData }
+ #[inline(never)]
+ #[cold]
+ fn arena_new_failed() -> ! {
+ panic!("Could not create a new UPB arena");
+ }
+
+ // SAFETY:
+ // - `upb_Arena_New` is assumed to be implemented correctly and always sound to
+ // call; if it returned a non-null pointer, it is a valid arena.
+ unsafe {
+ let Some(raw) = upb_Arena_New() else { arena_new_failed() };
+ Self { raw, _not_sync: PhantomData }
+ }
}
/// Returns the raw, UPB-managed pointer to the arena.
@@ -90,34 +104,54 @@
///
/// # Safety
///
- /// `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`.
+ /// - `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`.
#[inline]
pub unsafe fn alloc(&self, layout: Layout) -> &mut [MaybeUninit<u8>] {
debug_assert!(layout.align() <= UPB_MALLOC_ALIGN);
- let ptr = upb_Arena_Malloc(self.raw, layout.size());
+ // SAFETY: `self.raw` is a valid UPB arena
+ let ptr = unsafe { upb_Arena_Malloc(self.raw, layout.size()) };
if ptr.is_null() {
alloc::handle_alloc_error(layout);
}
- slice::from_raw_parts_mut(ptr.cast(), layout.size())
+ // SAFETY:
+ // - `upb_Arena_Malloc` promises that if the return pointer is non-null, it is
+ // dereferencable for `size` bytes and has an alignment of `UPB_MALLOC_ALIGN`
+ // until the arena is destroyed.
+ // - `[MaybeUninit<u8>]` has no alignment requirement, and `ptr` is aligned to a
+ // `UPB_MALLOC_ALIGN` boundary.
+ unsafe { slice::from_raw_parts_mut(ptr.cast(), layout.size()) }
}
/// Resizes some memory on the arena.
///
/// # Safety
///
- /// After calling this function, `ptr` is essentially zapped. `old` must
- /// be the layout `ptr` was allocated with via [`Arena::alloc()`]. `new`'s
- /// alignment must be less than `UPB_MALLOC_ALIGN`.
+ /// - `ptr` must be the data pointer returned by a previous call to `alloc`
+ /// or `resize` on `self`.
+ /// - After calling this function, `ptr` is no longer dereferencable - it is
+ /// zapped.
+ /// - `old` must be the layout `ptr` was allocated with via `alloc` or
+ /// `realloc`.
+ /// - `new`'s alignment must be less than `UPB_MALLOC_ALIGN`.
#[inline]
- pub unsafe fn resize(&self, ptr: *mut u8, old: Layout, new: Layout) -> &[MaybeUninit<u8>] {
+ pub unsafe fn resize(&self, ptr: *mut u8, old: Layout, new: Layout) -> &mut [MaybeUninit<u8>] {
debug_assert!(new.align() <= UPB_MALLOC_ALIGN);
- let ptr = upb_Arena_Realloc(self.raw, ptr, old.size(), new.size());
+ // SAFETY:
+ // - `self.raw` is a valid UPB arena
+ // - `ptr` was allocated by a previous call to `alloc` or `realloc` as promised
+ // by the caller.
+ let ptr = unsafe { upb_Arena_Realloc(self.raw, ptr, old.size(), new.size()) };
if ptr.is_null() {
alloc::handle_alloc_error(new);
}
- slice::from_raw_parts_mut(ptr.cast(), new.size())
+ // SAFETY:
+ // - `upb_Arena_Realloc` promises that if the return pointer is non-null, it is
+ // dereferencable for the new `size` in bytes until the arena is destroyed.
+ // - `[MaybeUninit<u8>]` has no alignment requirement, and `ptr` is aligned to a
+ // `UPB_MALLOC_ALIGN` boundary.
+ unsafe { slice::from_raw_parts_mut(ptr.cast(), new.size()) }
}
}
@@ -142,15 +176,28 @@
}
impl SerializedData {
+ /// Construct `SerializedData` from raw pointers and its owning arena.
+ ///
+ /// # Safety
+ /// - `arena` must be have allocated `data`
+ /// - `data` must be readable for `len` bytes and not mutate while this
+ /// struct exists
pub unsafe fn from_raw_parts(arena: Arena, data: NonNull<u8>, len: usize) -> Self {
SerializedData { _arena: arena, data, len }
}
+
+ /// Gets a raw slice pointer.
+ pub fn as_ptr(&self) -> *const [u8] {
+ ptr::slice_from_raw_parts(self.data.as_ptr(), self.len)
+ }
}
impl Deref for SerializedData {
type Target = [u8];
fn deref(&self) -> &Self::Target {
- unsafe { slice::from_raw_parts(self.data.as_ptr() as *const _, self.len) }
+ // SAFETY: `data` is valid for `len` bytes as promised by
+ // the caller of `SerializedData::from_raw_parts`.
+ unsafe { slice::from_raw_parts(self.data.as_ptr(), self.len) }
}
}
