DRAFT: Enabled clippy in CI to check for undocumented unsafety

.github/workflows/CI.yml

@@ -206,3 +206,15 @@ jobs:
 
       - name: cargo doc
         run: cargo rustdoc --features full,ffi -- --cfg docsrs --cfg hyper_unstable_ffi -D broken-intra-doc-links
+
+
+  clippy_check:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Find undocumented unsafe with clippy
+        run: cargo clippy --all-targets --features full -p hyper -- -A clippy::all -D clippy::undocumented_unsafe_blocks -D clippy::missing_safety_doc
+
+      - name: Run Clippy on hyper (but do not block CI)
+        run: cargo clippy --all-targets --features full -p hyper || true

clippy.toml

@@ -0,0 +1 @@
+msrv="1.56"

examples/client.rs

@@ -65,7 +65,7 @@ async fn fetch_url(url: hyper::Uri) -> Result<()> {
     while let Some(next) = res.frame().await {
         let frame = next?;
         if let Some(chunk) = frame.data_ref() {
-            io::stdout().write_all(&chunk).await?;
+            io::stdout().write_all(chunk).await?;
         }
     }
 

examples/gateway.rs

@@ -11,7 +11,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
     let in_addr: SocketAddr = ([127, 0, 0, 1], 3001).into();
     let out_addr: SocketAddr = ([127, 0, 0, 1], 3000).into();
 
-    let out_addr_clone = out_addr.clone();
+    let out_addr_clone = out_addr;
 
     let listener = TcpListener::bind(in_addr).await?;
 

examples/http_proxy.rs

@@ -106,7 +106,7 @@ async fn proxy(
 }
 
 fn host_addr(uri: &http::Uri) -> Option<String> {
-    uri.authority().and_then(|auth| Some(auth.to_string()))
+    uri.authority().map(|auth| auth.to_string())
 }
 
 fn empty() -> BoxBody<Bytes, hyper::Error> {

examples/upgrades.rs

@@ -160,7 +160,6 @@ async fn main() {
                             res = &mut conn => {
                                 if let Err(err) = res {
                                     println!("Error serving connection: {:?}", err);
-                                    return;
                                 }
                             }
                             // Continue polling the connection after enabling graceful shutdown.
@@ -178,7 +177,7 @@ async fn main() {
     });
 
     // Client requests a HTTP connection upgrade.
-    let request = client_upgrade_request(addr.clone());
+    let request = client_upgrade_request(addr);
     if let Err(e) = request.await {
         eprintln!("client error: {}", e);
     }

src/ext/h1_reason_phrase.rs

@@ -52,8 +52,10 @@ impl ReasonPhrase {
 
     /// Converts a `Bytes` directly into a `ReasonPhrase` without validating.
     ///
+    /// ## Safety
+    ///
     /// Use with care; invalid bytes in a reason phrase can cause serious security problems if
-    /// emitted in a response.
+    /// emitted in a response. The caller must make sure that `reason` is valid UTF-8.
     pub unsafe fn from_bytes_unchecked(reason: Bytes) -> Self {
         Self(reason)
     }

src/lib.rs

@@ -1,5 +1,7 @@
 #![deny(missing_docs)]
 #![deny(missing_debug_implementations)]
+#![deny(clippy::missing_safety_doc, clippy::undocumented_unsafe_blocks)]
+#![allow(clippy::type_complexity, clippy::manual_non_exhaustive, clippy::new_without_default)]
 #![cfg_attr(test, deny(rust_2018_idioms))]
 #![cfg_attr(all(test, feature = "full"), deny(unreachable_pub))]
 #![cfg_attr(all(test, feature = "full"), deny(warnings))]

src/proto/h1/conn.rs

@@ -949,11 +949,7 @@ impl State {
     }
 
     fn wants_keep_alive(&self) -> bool {
-        if let KA::Disabled = self.keep_alive.status() {
-            false
-        } else {
-            true
-        }
+        !matches!(self.keep_alive.status(), KA::Disabled)
     }
 
     fn try_keep_alive<T: Http1Transaction>(&mut self) {

src/proto/h1/io.rs

@@ -249,16 +249,17 @@ where
         }
 
         let dst = self.read_buf.chunk_mut();
+        // Safety: treat the chunk as array of MaybeUninit bytes
         let dst = unsafe { &mut *(dst as *mut _ as *mut [MaybeUninit<u8>]) };
         let mut buf = ReadBuf::uninit(dst);
         match Pin::new(&mut self.io).poll_read(cx, &mut buf) {
             Poll::Ready(Ok(_)) => {
                 let n = buf.filled().len();
                 trace!("received {} bytes", n);
+                // Safety: we just read that many bytes into the
+                // uninitialized part of the buffer, so this is okay.
+                // @tokio pls give me back `poll_read_buf` thanks
                 unsafe {
-                    // Safety: we just read that many bytes into the
-                    // uninitialized part of the buffer, so this is okay.
-                    // @tokio pls give me back `poll_read_buf` thanks
                     self.read_buf.advance_mut(n);
                 }
                 self.read_buf_strategy.record(n);

src/proto/h1/role.rs

@@ -43,6 +43,8 @@ macro_rules! header_name {
 macro_rules! header_value {
     ($bytes:expr) => {{
         {
+            // SAFETY: this should be checked in previous places.
+            #[allow(clippy::undocumented_unsafe_blocks)]
             unsafe { HeaderValue::from_maybe_shared_unchecked($bytes) }
         }
     }};
@@ -132,7 +134,7 @@ impl Http1Transaction for Server {
         let len;
         let headers_len;
 
-        // Unsafe: both headers_indices and headers are using uninitialized memory,
+        // SAFETY: both headers_indices and headers are using uninitialized memory,
         // but we *never* read any of it until after httparse has assigned
         // values into it. By not zeroing out the stack memory, this saves
         // a good ~5% on pipeline benchmarks.
@@ -141,8 +143,8 @@ impl Http1Transaction for Server {
             MaybeUninit::uninit().assume_init()
         };
         {
-            /* SAFETY: it is safe to go from MaybeUninit array to array of MaybeUninit */
             let mut headers: [MaybeUninit<httparse::Header<'_>>; MAX_HEADERS] =
+                // SAFETY: it is safe to go from MaybeUninit array to array of MaybeUninit
                 unsafe { MaybeUninit::uninit().assume_init() };
             trace!(bytes = buf.len(), "Request.parse");
             let mut req = httparse::Request::new(&mut []);
@@ -169,7 +171,7 @@ impl Http1Transaction for Server {
                         Version::HTTP_10
                     };
 
-                    record_header_indices(bytes, &req.headers, &mut headers_indices)?;
+                    record_header_indices(bytes, req.headers, &mut headers_indices)?;
                     headers_len = req.headers.len();
                 }
                 Ok(httparse::Status::Partial) => return Ok(None),
@@ -452,9 +454,10 @@ impl Http1Transaction for Server {
         };
 
         debug!("sending automatic response ({}) for parse error", status);
-        let mut msg = MessageHead::default();
-        msg.subject = status;
-        Some(msg)
+        Some(MessageHead {
+            subject: status,
+            ..Default::default()
+        })
     }
 
     fn is_server() -> bool {
@@ -479,21 +482,15 @@ impl Server {
         } else if status.is_informational() {
             false
         } else {
-            match status {
-                StatusCode::NO_CONTENT | StatusCode::NOT_MODIFIED => false,
-                _ => true,
-            }
+            !matches!(status, StatusCode::NO_CONTENT | StatusCode::NOT_MODIFIED)
         }
     }
 
     fn can_have_content_length(method: &Option<Method>, status: StatusCode) -> bool {
         if status.is_informational() || method == &Some(Method::CONNECT) && status.is_success() {
             false
         } else {
-            match status {
-                StatusCode::NO_CONTENT | StatusCode::NOT_MODIFIED => false,
-                _ => true,
-            }
+            !matches!(status, StatusCode::NO_CONTENT | StatusCode::NOT_MODIFIED)
         }
     }
 
@@ -933,14 +930,14 @@ impl Http1Transaction for Client {
 
         // Loop to skip information status code headers (100 Continue, etc).
         loop {
-            // Unsafe: see comment in Server Http1Transaction, above.
+            // SAFETY: see comment in Server Http1Transaction, above.
             let mut headers_indices: [MaybeUninit<HeaderIndices>; MAX_HEADERS] = unsafe {
                 // SAFETY: We can go safely from MaybeUninit array to array of MaybeUninit
                 MaybeUninit::uninit().assume_init()
             };
             let (len, status, reason, version, headers_len) = {
-                // SAFETY: We can go safely from MaybeUninit array to array of MaybeUninit
                 let mut headers: [MaybeUninit<httparse::Header<'_>>; MAX_HEADERS] =
+                    // SAFETY: We can go safely from MaybeUninit array to array of MaybeUninit
                     unsafe { MaybeUninit::uninit().assume_init() };
                 trace!(bytes = buf.len(), "Response.parse");
                 let mut res = httparse::Response::new(&mut []);

src/proto/h2/client.rs

@@ -13,8 +13,8 @@ use tracing::{debug, trace, warn};
 
 use super::{ping, H2Upgraded, PipeToSendStream, SendBuf};
 use crate::body::{Body, Incoming as IncomingBody};
-use crate::common::time::Time;
 use crate::client::dispatch::Callback;
+use crate::common::time::Time;
 use crate::common::{exec::Exec, task, Future, Never, Pin, Poll};
 use crate::ext::Protocol;
 use crate::headers;
@@ -128,7 +128,7 @@ where
         }
     });
 
-    let ping_config = new_ping_config(&config);
+    let ping_config = new_ping_config(config);
 
     let (conn, ping) = if ping_config.is_enabled() {
         let pp = conn.ping_pong().expect("conn.ping_pong");
@@ -340,14 +340,11 @@ where
                 }
             };
 
-            match self.fut_ctx.take() {
+            if let Some(f) = self.fut_ctx.take() {
                 // If we were waiting on pending open
                 // continue where we left off.
-                Some(f) => {
-                    self.poll_pipe(f, cx);
-                    continue;
-                }
-                None => (),
+                self.poll_pipe(f, cx);
+                continue;
             }
 
             match self.req_rx.poll_recv(cx) {

src/upgrade.rs

@@ -282,6 +282,7 @@ impl dyn Io + Send {
     fn __hyper_downcast<T: Io>(self: Box<Self>) -> Result<Box<T>, Box<Self>> {
         if self.__hyper_is::<T>() {
             // Taken from `std::error::Error::downcast()`.
+            // SAFETY: downcasting requires unsafe, but is checked with __hyper_is to be safe.
             unsafe {
                 let raw: *mut dyn Io = Box::into_raw(self);
                 Ok(Box::from_raw(raw as *mut T))

tests/client.rs

@@ -997,7 +997,7 @@ test! {
             \r\n\
             ",
         reply: {
-            let long_header = std::iter::repeat("A").take(500_000).collect::<String>();
+            let long_header = "A".repeat(500_000);
             format!("\
                 HTTP/1.1 200 OK\r\n\
                 {}: {}\r\n\

tests/server.rs

@@ -1589,7 +1589,7 @@ async fn upgrades_new() {
 
         let response = s(&buf);
         assert!(response.starts_with("HTTP/1.1 101 Switching Protocols\r\n"));
-        assert!(!has_header(&response, "content-length"));
+        assert!(!has_header(response, "content-length"));
         let _ = read_101_tx.send(());
 
         let n = tcp.read(&mut buf).expect("read 2");

tests/support/mod.rs

@@ -367,7 +367,7 @@ async fn async_test(cfg: __TestConfig) {
                 assert_eq!(req.method(), &sreq.method, "client method");
                 assert_eq!(req.version(), version, "client version");
                 for func in &sreq.headers {
-                    func(&req.headers());
+                    func(req.headers());
                 }
                 let sbody = sreq.body;
                 req.collect().map_ok(move |collected| {
@@ -455,7 +455,7 @@ async fn async_test(cfg: __TestConfig) {
             assert_eq!(res.status(), cstatus, "server status");
             assert_eq!(res.version(), version, "server version");
             for func in &cheaders {
-                func(&res.headers());
+                func(res.headers());
             }
 
             let body = res.collect().await.unwrap().to_bytes();