From 8e1b52eeb6ab447a9405f4f83c567e99cb57cfef Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Thu, 8 Jun 2023 12:53:20 +0000 Subject: [PATCH] Enabled clippy in CI; added some safety comments; fixed some clippy warnings; --- .github/workflows/ci.yml | 11 +++++++++++ Cargo.toml | 2 +- clippy.toml | 1 + src/extensions.rs | 1 + src/header/map.rs | 20 ++++++++++++++++---- src/header/name.rs | 3 ++- src/header/value.rs | 5 +++++ src/lib.rs | 4 ++++ src/status.rs | 6 +++++- src/uri/mod.rs | 3 +++ src/uri/path.rs | 1 + 11 files changed, 50 insertions(+), 7 deletions(-) create mode 100644 clippy.toml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 603cefec..0409fa33 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,17 @@ jobs: if: matrix.benches run: cargo test --benches ${{ matrix.features }} + + clippy_check: + runs-on: ubuntu-latest + # Make sure CI fails on all warnings, including Clippy lints + env: + RUSTFLAGS: "-Dwarnings" + steps: + - uses: actions/checkout@v3 + - name: Run Clippy + run: cargo clippy --all-targets --all-features + msrv: name: Test MSRV runs-on: ubuntu-latest diff --git a/Cargo.toml b/Cargo.toml index 75eaae57..8f364c5d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,7 @@ A set of types for representing HTTP requests and responses. keywords = ["http"] categories = ["web-programming"] edition = "2018" -# When updating this value, don't forget to also adjust the GitHub Actions config. +# When updating this value, don't forget to also adjust the clippy config. rust-version = "1.49.0" [workspace] diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..f3885f59 --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +msrv="1.49" diff --git a/src/extensions.rs b/src/extensions.rs index 7e815df7..942aa22e 100644 --- a/src/extensions.rs +++ b/src/extensions.rs @@ -60,6 +60,7 @@ impl Extensions { /// assert_eq!(ext.insert(9i32), Some(5i32)); /// ``` pub fn insert(&mut self, val: T) -> Option { + #[allow(clippy::box_default)] self.map .get_or_insert_with(|| Box::new(HashMap::default())) .insert(TypeId::of::(), Box::new(val)) diff --git a/src/header/map.rs b/src/header/map.rs index ed049dbc..a52d4e0a 100644 --- a/src/header/map.rs +++ b/src/header/map.rs @@ -961,6 +961,7 @@ impl HeaderMap { let entries = &mut self.entries[..] as *mut _; let extra_values = &mut self.extra_values as *mut _; let len = self.entries.len(); + // SAFETY: see comment above unsafe { self.entries.set_len(0); } Drain { @@ -2070,6 +2071,7 @@ impl<'a, T> Iterator for Iter<'a, T> { fn next(&mut self) -> Option { self.inner .next_unsafe() + // SAFETY: Iter invariant: only valid pointers .map(|(key, ptr)| (key, unsafe { &*ptr })) } @@ -2090,6 +2092,7 @@ impl<'a, T> IterMut<'a, T> { use self::Cursor::*; if self.cursor.is_none() { + // SAFETY: invariant dereferencing the self.map pointer is always safe if (self.entry + 1) >= unsafe { &*self.map }.entries.len() { return None; } @@ -2098,6 +2101,7 @@ impl<'a, T> IterMut<'a, T> { self.cursor = Some(Cursor::Head); } + // SAFETY: invariant dereferencing the self.map pointer is always safe let entry = unsafe { &(*self.map).entries[self.entry] }; match self.cursor.unwrap() { @@ -2106,6 +2110,7 @@ impl<'a, T> IterMut<'a, T> { Some((&entry.key, &entry.value as *const _ as *mut _)) } Values(idx) => { + // SAFETY: invariant dereferencing the self.map pointer is always safe let extra = unsafe { &(*self.map).extra_values[idx] }; match extra.next { @@ -2128,6 +2133,7 @@ impl<'a, T> Iterator for IterMut<'a, T> { } fn size_hint(&self) -> (usize, Option) { + // SAFETY: invariant dereferencing the self.map pointer is always safe let map = unsafe { &*self.map }; debug_assert!(map.entries.len() >= self.entry); @@ -2204,9 +2210,8 @@ impl<'a, T> Iterator for Drain<'a, T> { // Remove the extra value let raw_links = RawLinks(self.entries); - let extra = unsafe { - remove_extra_value(raw_links, &mut *self.extra_values, next) - }; + // SAFETY: dereferencing self.extra_values valid as long as self is alive. + let extra = remove_extra_value(raw_links, unsafe { &mut *self.extra_values } , next); match extra.next { Link::Extra(idx) => self.next = Some(idx), @@ -2224,6 +2229,8 @@ impl<'a, T> Iterator for Drain<'a, T> { self.idx += 1; + // SAFETY: pointer operation always valid, as `self` cannot outlive the HeaderMap it is + // referencing. unsafe { let entry = &(*self.entries)[idx]; @@ -2243,6 +2250,7 @@ impl<'a, T> Iterator for Drain<'a, T> { // For instance, extending a new `HeaderMap` wouldn't need to // reserve the upper-bound in `entries`, only the lower-bound. let lower = self.len - self.idx; + // SAFETY: dereferencing self.extra_values valid as long as self is alive. let upper = unsafe { (*self.extra_values).len() } + lower; (lower, Some(upper)) } @@ -2606,6 +2614,7 @@ impl<'a, T: 'a> Iterator for ValueIterMut<'a, T> { fn next(&mut self) -> Option { use self::Cursor::*; + // SAFETY: dereferencing self.map valid as long as self is alive. let entry = unsafe { &mut (*self.map).entries[self.index] }; match self.front { @@ -2626,6 +2635,7 @@ impl<'a, T: 'a> Iterator for ValueIterMut<'a, T> { Some(&mut entry.value) } Some(Values(idx)) => { + // SAFETY: dereferencing self.map valid as long as self is alive. let extra = unsafe { &mut (*self.map).extra_values[idx] }; if self.front == self.back { @@ -2649,6 +2659,7 @@ impl<'a, T: 'a> DoubleEndedIterator for ValueIterMut<'a, T> { fn next_back(&mut self) -> Option { use self::Cursor::*; + // SAFETY: dereferencing self.map valid as long as self is alive. let entry = unsafe { &mut (*self.map).entries[self.index] }; match self.back { @@ -2658,6 +2669,7 @@ impl<'a, T: 'a> DoubleEndedIterator for ValueIterMut<'a, T> { Some(&mut entry.value) } Some(Values(idx)) => { + // SAFETY: dereferencing self.map valid as long as self is alive. let extra = unsafe { &mut (*self.map).extra_values[idx] }; if self.front == self.back { @@ -2726,7 +2738,7 @@ impl Drop for IntoIter { // Ensure the iterator is consumed for _ in self.by_ref() {} - // All the values have already been yielded out. + // SAFETY: All the values have already been yielded out, once dropped. unsafe { self.extra_values.set_len(0); } diff --git a/src/header/name.rs b/src/header/name.rs index 6080cf08..5d32272d 100644 --- a/src/header/name.rs +++ b/src/header/name.rs @@ -87,8 +87,8 @@ macro_rules! standard_headers { #[inline] fn as_str(&self) -> &'static str { match *self { - // Safety: test_parse_standard_headers ensures these &[u8]s are &str-safe. $( + // SAFETY: test_parse_standard_headers ensures these &[u8]s are &str-safe. StandardHeader::$konst => unsafe { std::str::from_utf8_unchecked( $name_bytes ) }, )+ } @@ -1267,6 +1267,7 @@ impl HeaderName { i += 1; } } { + #[allow(clippy::no_effect)] ([] as [u8; 0])[0]; // Invalid header name } diff --git a/src/header/value.rs b/src/header/value.rs index bf05f16f..bca69628 100644 --- a/src/header/value.rs +++ b/src/header/value.rs @@ -85,6 +85,7 @@ impl HeaderValue { let mut i = 0; while i < bytes.len() { if !is_visible_ascii(bytes[i]) { + #[allow(clippy::no_effect)] ([] as [u8; 0])[0]; // Invalid header value } i += 1; @@ -191,6 +192,10 @@ impl HeaderValue { /// /// This function does NOT validate that illegal bytes are not contained /// within the buffer. + /// + /// ## Safety + /// + /// The caller must ensure that `src` contains only legal utf-8. pub unsafe fn from_maybe_shared_unchecked(src: T) -> HeaderValue where T: AsRef<[u8]> + 'static, diff --git a/src/lib.rs b/src/lib.rs index 38a4c2aa..edfca9c6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,6 +159,10 @@ //! ``` #![deny(warnings, missing_docs, missing_debug_implementations)] +#![deny( + clippy::missing_safety_doc, + clippy::undocumented_unsafe_blocks +)] #[cfg(test)] #[macro_use] diff --git a/src/status.rs b/src/status.rs index d98d24c3..abdadd25 100644 --- a/src/status.rs +++ b/src/status.rs @@ -143,6 +143,8 @@ impl StatusCode { { &CODE_DIGITS[offset..offset+3] } #[cfg(not(debug_assertions))] + // SAFETY: we assume `StatusCode` is constructed in a way that self.0 (the numerical status + // code is >= 100 and also <= 1000, which makes any possible offset here valid. unsafe { CODE_DIGITS.get_unchecked(offset..offset+3) } } @@ -304,7 +306,9 @@ macro_rules! status_codes { impl StatusCode { $( $(#[$docs])* - pub const $konst: StatusCode = StatusCode(unsafe { NonZeroU16::new_unchecked($num) }); + pub const $konst: StatusCode = StatusCode( + // SAFETY: only called with constants + unsafe { NonZeroU16::new_unchecked($num) }); )+ } diff --git a/src/uri/mod.rs b/src/uri/mod.rs index 30be83b5..8cb6e216 100644 --- a/src/uri/mod.rs +++ b/src/uri/mod.rs @@ -837,6 +837,7 @@ fn parse_full(mut s: Bytes) -> Result { let _ = scheme.split_off(n); // Allocate the ByteStr + // SAFETY: previously verified by `Scheme2::parse` let val = unsafe { ByteStr::from_utf8_unchecked(scheme) }; Scheme2::Other(Box::new(val)) @@ -853,6 +854,7 @@ fn parse_full(mut s: Bytes) -> Result { } let authority = Authority { + // SAFETY: previously verified by `Authority::parse` data: unsafe { ByteStr::from_utf8_unchecked(s) }, }; @@ -870,6 +872,7 @@ fn parse_full(mut s: Bytes) -> Result { let authority = s.split_to(authority_end); let authority = Authority { + // SAFETY: previously verified by `Authority::parse` data: unsafe { ByteStr::from_utf8_unchecked(authority) }, }; diff --git a/src/uri/path.rs b/src/uri/path.rs index be2cb65c..0c390157 100644 --- a/src/uri/path.rs +++ b/src/uri/path.rs @@ -97,6 +97,7 @@ impl PathAndQuery { } Ok(PathAndQuery { + // Safety: previous iteration ensures that src is also valid utf-8 data: unsafe { ByteStr::from_utf8_unchecked(src) }, query: query, })