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

#![deny(clippy::undocumented_unsafe_blocks)]: Add and fix existing safety comments #1305

Merged
merged 4 commits into from
Jul 11, 2024
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
6 changes: 4 additions & 2 deletions include/dav1d/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ impl From<Dav1dData> for Rav1dData {
m,
} = value;
Self {
// Safety: `r#ref` is a [`RawCArc`] originally from [`CArc`].
data: r#ref.map(|r#ref| unsafe { CArc::from_raw(r#ref) }),
data: r#ref.map(|r#ref| {
// SAFETY: `r#ref` is a [`RawCArc`] originally from [`CArc`].
unsafe { CArc::from_raw(r#ref) }
}),
m: m.into(),
}
}
Expand Down
40 changes: 26 additions & 14 deletions include/dav1d/picture.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,24 +449,36 @@ impl From<Dav1dPicture> for Rav1dPicture {
} = value;
Self {
// We don't `.update_rav1d()` [`Rav1dSequenceHeader`] because it's meant to be read-only.
// Safety: `raw` came from [`RawArc::from_arc`].
seq_hdr: seq_hdr_ref.map(|raw| unsafe { raw.into_arc() }),
seq_hdr: seq_hdr_ref.map(|raw| {
// SAFETY: `raw` came from [`RawArc::from_arc`].
unsafe { raw.into_arc() }
}),
// We don't `.update_rav1d()` [`Rav1dFrameHeader`] because it's meant to be read-only.
// Safety: `raw` came from [`RawArc::from_arc`].
frame_hdr: frame_hdr_ref.map(|raw| unsafe { raw.into_arc() }),
// Safety: `raw` came from [`RawArc::from_arc`].
data: data_ref.map(|raw| unsafe { raw.into_arc() }),
frame_hdr: frame_hdr_ref.map(|raw| {
// SAFETY: `raw` came from [`RawArc::from_arc`].
unsafe { raw.into_arc() }
}),
data: data_ref.map(|raw| {
// SAFETY: `raw` came from [`RawArc::from_arc`].
unsafe { raw.into_arc() }
}),
stride,
p: p.into(),
m: m.into(),
// Safety: `raw` came from [`RawArc::from_arc`].
content_light: content_light_ref.map(|raw| unsafe { raw.into_arc() }),
// Safety: `raw` came from [`RawArc::from_arc`].
mastering_display: mastering_display_ref.map(|raw| unsafe { raw.into_arc() }),
content_light: content_light_ref.map(|raw| {
// SAFETY: `raw` came from [`RawArc::from_arc`].
unsafe { raw.into_arc() }
}),
mastering_display: mastering_display_ref.map(|raw| {
// Safety: `raw` came from [`RawArc::from_arc`].
unsafe { raw.into_arc() }
}),
// We don't `.update_rav1d` [`Rav1dITUTT35`] because never read it.
// Safety: `raw` came from [`RawArc::from_arc`].
itut_t35: itut_t35_ref
.map(|raw| unsafe { raw.into_arc() })
.map(|raw| {
// SAFETY: `raw` came from [`RawArc::from_arc`].
unsafe { raw.into_arc() }
})
.unwrap_or_default(),
}
}
Expand Down Expand Up @@ -740,7 +752,7 @@ impl Rav1dPicAllocator {
..Default::default()
};
let mut pic_c = pic.to::<Dav1dPicture>();
// Safety: `pic_c` is a valid `Dav1dPicture` with `data`, `stride`, `allocator_data` unset.
// SAFETY: `pic_c` is a valid `Dav1dPicture` with `data`, `stride`, `allocator_data` unset.
let result = unsafe { (self.alloc_picture_callback)(&mut pic_c, self.cookie) };
result.try_to::<Rav1dResult>().unwrap()?;
// `data`, `stride`, and `allocator_data` are the only fields set by the allocator.
Expand Down Expand Up @@ -777,7 +789,7 @@ impl Rav1dPicAllocator {
allocator_data,
..Default::default()
};
// Safety: `pic_c` contains the same `data` and `allocator_data`
// SAFETY: `pic_c` contains the same `data` and `allocator_data`
// that `Self::alloc_picture_data` set, which now get deallocated here.
unsafe {
(self.release_picture_callback)(&mut pic_c, self.cookie);
Expand Down
1 change: 1 addition & 0 deletions lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
)]
#![deny(unsafe_op_in_unsafe_fn)]
#![allow(clippy::all)]
#![deny(clippy::undocumented_unsafe_blocks)]

#[cfg(not(any(feature = "bitdepth_8", feature = "bitdepth_16")))]
compile_error!("No bitdepths enabled. Enable one or more of the following features: `bitdepth_8`, `bitdepth_16`");
Expand Down
4 changes: 2 additions & 2 deletions src/align.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,14 +169,14 @@ impl<T: Copy, C: AlignedByteChunk> AlignedVec<T, C> {

/// Extract a slice containing the entire vector.
pub fn as_slice(&self) -> &[T] {
// Safety: The first `len` elements have been
// SAFETY: The first `len` elements have been
// initialized to `T`s in `Self::resize_with`.
unsafe { slice::from_raw_parts(self.as_ptr(), self.len) }
}

/// Extract a mutable slice of the entire vector.
pub fn as_mut_slice(&mut self) -> &mut [T] {
// Safety: The first `len` elements have been
// SAFETY: The first `len` elements have been
// initialized to `T`s in `Self::resize_with`.
unsafe { slice::from_raw_parts_mut(self.as_mut_ptr(), self.len) }
}
Expand Down
6 changes: 3 additions & 3 deletions src/c_arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use std::sync::Arc;

pub fn arc_into_raw<T: ?Sized>(arc: Arc<T>) -> NonNull<T> {
let raw = Arc::into_raw(arc).cast_mut();
// Safety: [`Arc::into_raw`] never returns null.
// SAFETY: [`Arc::into_raw`] never returns null.
unsafe { NonNull::new_unchecked(raw) }
}

Expand Down Expand Up @@ -106,7 +106,7 @@ impl<T: ?Sized> AsRef<T> for CArc<T> {
}
}

// Safety: [`Self::stable_ref`] is a ptr
// SAFETY: [`Self::stable_ref`] is a ptr
// derived from [`Self::owner`]'s through [`CBox::as_ref`]
// and is thus safe to dereference.
// The [`CBox`] is [`Pin`]ned and
Expand Down Expand Up @@ -239,7 +239,7 @@ impl<T: ?Sized> CArc<T> {
///
/// The [`RawCArc`] must be originally from [`Self::into_raw`].
pub unsafe fn from_raw(raw: RawCArc<T>) -> Self {
// Safety: The [`RawCArc`] contains the output of [`Arc::into_raw`],
// SAFETY: The [`RawCArc`] contains the output of [`Arc::into_raw`],
// so we can call [`Arc::from_raw`] on it.
let owner = unsafe { raw.0.into_arc() };
owner.into()
Expand Down
8 changes: 4 additions & 4 deletions src/c_box.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ unsafe impl<T: Sync + ?Sized> Sync for Unique<T> {}
pub enum CBox<T: ?Sized> {
Rust(Box<T>),
C {
/// # Safety:
/// # SAFETY:
///
/// * Never moved.
/// * Valid to dereference.
Expand Down Expand Up @@ -103,12 +103,12 @@ impl<T: ?Sized> Drop for CBox<T> {
Self::Rust(_) => {} // Drop normally.
Self::C { data, free, .. } => {
let ptr = data.pointer.as_ptr();
// Safety: See below.
// SAFETY: See below.
// The [`FnFree`] won't run Rust's `fn drop`,
// so we have to do this ourselves first.
unsafe { drop_in_place(ptr) };
let ptr = ptr.cast();
// Safety: See safety docs on [`Self::data`] and [`Self::from_c`].
// SAFETY: See safety docs on [`Self::data`] and [`Self::from_c`].
unsafe { free.free(ptr) }
}
}
Expand Down Expand Up @@ -137,7 +137,7 @@ impl<T: ?Sized> CBox<T> {
}

pub fn into_pin(self) -> Pin<Self> {
// Safety:
// SAFETY:
// If `self` is `Self::Rust`, `Box` can be pinned.
// If `self` is `Self::C`, `data` is never moved until [`Self::drop`].
unsafe { Pin::new_unchecked(self) }
Expand Down
4 changes: 4 additions & 0 deletions src/disjoint_mut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,7 @@ impl<V: Copy, C: AlignedByteChunk> DisjointMut<AlignedVec<V, C>> {
}
}

#[allow(clippy::undocumented_unsafe_blocks)]
#[test]
fn test_overlapping_immut() {
let mut v: DisjointMut<Vec<u8>> = Default::default();
Expand All @@ -1013,6 +1014,7 @@ fn test_overlapping_immut() {
assert_eq!(guard1[2], guard2[0]);
}

#[allow(clippy::undocumented_unsafe_blocks)]
#[test]
#[cfg_attr(debug_assertions, should_panic)]
fn test_overlapping_mut() {
Expand All @@ -1026,6 +1028,7 @@ fn test_overlapping_mut() {
assert_eq!(guard1[2], 42);
}

#[allow(clippy::undocumented_unsafe_blocks)]
#[cfg(debug_assertions)]
#[test]
fn test_pointer_write_debug() {
Expand All @@ -1052,6 +1055,7 @@ fn test_pointer_write_debug() {

// Run with miri using the following command:
// RUSTFLAGS="-C debug-assertions=off" cargo miri test
#[allow(clippy::undocumented_unsafe_blocks)]
#[cfg(not(debug_assertions))]
#[test]
fn test_pointer_write_release() {
Expand Down
25 changes: 12 additions & 13 deletions src/filmgrain.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
#![deny(unsafe_op_in_unsafe_fn)]
#![warn(clippy::all)]
#![allow(clippy::too_many_arguments)]

use crate::include::common::bitdepth::AsPrimitive;
use crate::include::common::bitdepth::BitDepth;
Expand Down Expand Up @@ -288,7 +286,7 @@ unsafe extern "C" fn generate_grain_y_c_erased<BD: BitDepth>(
data: &Dav1dFilmGrainData,
bitdepth_max: c_int,
) {
// Safety: Casting back to the original type from the `generate_grain_y::Fn::call`.
// SAFETY: Casting back to the original type from the `generate_grain_y::Fn::call`.
let buf = unsafe { &mut *buf.cast() };
let data = &data.clone().into();
let bd = BD::from_c(bitdepth_max);
Expand Down Expand Up @@ -448,7 +446,7 @@ fn generate_grain_uv_rust<BD: BitDepth>(
let (luma_y, luma_x) = is_sub.luma((y, x));
const _: () = IsSub::check_buf_index_all(&None::<GrainLut<()>>);
// The optimizer is not smart enough to deduce this on its own.
// Safety: The above static check checks all maximum index possibilities.
// SAFETY: The above static check checks all maximum index possibilities.
unsafe {
assume(luma_y < GRAIN_HEIGHT + 1 - 1);
assume(luma_x < GRAIN_WIDTH - 1);
Expand Down Expand Up @@ -492,9 +490,9 @@ unsafe extern "C" fn generate_grain_uv_c_erased<
uv: intptr_t,
bitdepth_max: c_int,
) {
// Safety: Casting back to the original type from the `generate_grain_uv::Fn::call`.
// SAFETY: Casting back to the original type from the `generate_grain_uv::Fn::call`.
let buf = unsafe { &mut *buf.cast() };
// Safety: Casting back to the original type from the `generate_grain_uv::Fn::call`.
// SAFETY: Casting back to the original type from the `generate_grain_uv::Fn::call`.
let buf_y = unsafe { &*buf_y.cast() };
let data = &data.clone().into();
let is_uv = uv != 0;
Expand Down Expand Up @@ -546,9 +544,9 @@ unsafe extern "C" fn fgy_32x32xn_c_erased<BD: BitDepth>(
// SAFETY: Was passed as `FFISafe::new(_)` in `fgy_32x32xn::Fn::call`.
let [dst_row, src_row] = [dst_row, src_row].map(|it| *unsafe { FFISafe::get(it) });
let data = &data.clone().into();
// Safety: Casting back to the original type from the `fn` ptr call.
// SAFETY: Casting back to the original type from the `fn` ptr call.
let scaling = unsafe { &*scaling.cast() };
// Safety: Casting back to the original type from the `fn` ptr call.
// SAFETY: Casting back to the original type from the `fn` ptr call.
let grain_lut = unsafe { &*grain_lut.cast() };
let bh = bh as usize;
let row_num = row_num as usize;
Expand Down Expand Up @@ -883,13 +881,14 @@ unsafe extern "C" fn fguv_32x32xn_c_erased<
src_row: *const FFISafe<Rav1dPictureDataComponentOffset>,
luma_row: *const FFISafe<Rav1dPictureDataComponentOffset>,
) {
// SAFETY: Was passed as `FFISafe::new(_)` in `fguv_32x32xn::Fn::call`.
let [dst_row, src_row, luma_row] =
[dst_row, src_row, luma_row].map(|row| *unsafe { FFISafe::get(row) });
let [dst_row, src_row, luma_row] = [dst_row, src_row, luma_row].map(|row| {
// SAFETY: Was passed as `FFISafe::new(_)` in `fguv_32x32xn::Fn::call`.
*unsafe { FFISafe::get(row) }
});
let data = &data.clone().into();
// Safety: Casting back to the original type from the `fn` ptr call.
// SAFETY: Casting back to the original type from the `fn` ptr call.
let scaling = unsafe { &*scaling.cast() };
// Safety: Casting back to the original type from the `fn` ptr call.
// SAFETY: Casting back to the original type from the `fn` ptr call.
let grain_lut = unsafe { &*grain_lut.cast() };
let bh = bh as usize;
let row_num = row_num as usize;
Expand Down
2 changes: 1 addition & 1 deletion src/intra_edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ impl<const SB128: bool, const N_BRANCH: usize, const N_TIP: usize>
if cfg!(debug_assertions) {
&edges[edge.index as usize]
} else {
// Safety: Already checked in `Self::check_indices`, and `EdgeIndex`'s fields are private.
// SAFETY: Already checked in `Self::check_indices`, and `EdgeIndex`'s fields are private.
unsafe { edges.get_unchecked(edge.index as usize) }
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/lf_mask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ fn mask_edges_inter(
}

for y in 0..h4 {
// SAFETY y < h4 and w4 - 1 < w4 so txa[0][0][y][w4 - 1] is initialized.
// SAFETY: y < h4 and w4 - 1 < w4 so txa[0][0][y][w4 - 1] is initialized.
l[y] = unsafe { txa[0][0][y][w4 - 1].assume_init() };
}
// SAFETY: h4 - 1 < h4 and ..w4 < w4 so txa[1][0][h4 - 1][..w4] is
Expand Down
4 changes: 2 additions & 2 deletions src/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl fmt::Write for Dav1dLogger {
// or the Rust API can be used instead.
let fmt = c"%c";
for &byte in s.as_bytes() {
// # Safety
// SAFETY:
//
// The first argument is `self.cookie`
// and the rest are safe to call `printf` with,
Expand Down Expand Up @@ -120,7 +120,7 @@ mod marker {
type Callback = extern "C" fn(cookie: *mut c_void, fmt: *const c_char);

const fn cast(callback: Callback) -> Dav1dLoggerCallback {
// Safety: It should always be safe to ignore variadic args.
// SAFETY: It should always be safe to ignore variadic args.
// Declaring a variadic `fn` is unstable, though, which is why we avoid that.
unsafe { std::mem::transmute(callback) }
}
Expand Down
Loading