diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index bfe5460bc6..446ac2055b 100644 --- a/.github/workflows/CI.yml +++ b/.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 diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000000..72a8526886 --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +msrv="1.56" diff --git a/examples/client.rs b/examples/client.rs index ffcc026719..dd5a004103 100644 --- a/examples/client.rs +++ b/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?; } } diff --git a/examples/gateway.rs b/examples/gateway.rs index 907f2fdba2..623408e400 100644 --- a/examples/gateway.rs +++ b/examples/gateway.rs @@ -11,7 +11,7 @@ async fn main() -> Result<(), Box> { 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?; diff --git a/examples/http_proxy.rs b/examples/http_proxy.rs index 0b4a6818b8..48cc5a67e4 100644 --- a/examples/http_proxy.rs +++ b/examples/http_proxy.rs @@ -106,7 +106,7 @@ async fn proxy( } fn host_addr(uri: &http::Uri) -> Option { - uri.authority().and_then(|auth| Some(auth.to_string())) + uri.authority().map(|auth| auth.to_string()) } fn empty() -> BoxBody { diff --git a/examples/upgrades.rs b/examples/upgrades.rs index 92a80d7567..497b6ccb1b 100644 --- a/examples/upgrades.rs +++ b/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); } diff --git a/src/ext/h1_reason_phrase.rs b/src/ext/h1_reason_phrase.rs index 021b632b6d..51a90003ec 100644 --- a/src/ext/h1_reason_phrase.rs +++ b/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) } diff --git a/src/lib.rs b/src/lib.rs index 054beb225a..65de326c18 100644 --- a/src/lib.rs +++ b/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))] diff --git a/src/proto/h1/conn.rs b/src/proto/h1/conn.rs index b7c619683c..d2bc5fc399 100644 --- a/src/proto/h1/conn.rs +++ b/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(&mut self) { diff --git a/src/proto/h1/io.rs b/src/proto/h1/io.rs index da4101b6fb..96b3209fc2 100644 --- a/src/proto/h1/io.rs +++ b/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]) }; 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); diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 6a1c5a87d3..5e71d4814e 100644 --- a/src/proto/h1/role.rs +++ b/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>; 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,10 +482,7 @@ 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) } } @@ -490,10 +490,7 @@ impl Server { 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; 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>; 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 []); diff --git a/src/proto/h2/client.rs b/src/proto/h2/client.rs index 121e24dd84..0659d12318 100644 --- a/src/proto/h2/client.rs +++ b/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) { diff --git a/src/upgrade.rs b/src/upgrade.rs index 1c7b5b01cd..ea82d67c5c 100644 --- a/src/upgrade.rs +++ b/src/upgrade.rs @@ -282,6 +282,7 @@ impl dyn Io + Send { fn __hyper_downcast(self: Box) -> Result, Box> { if self.__hyper_is::() { // 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)) diff --git a/tests/client.rs b/tests/client.rs index 842282c5bb..e8dfaf5303 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -997,7 +997,7 @@ test! { \r\n\ ", reply: { - let long_header = std::iter::repeat("A").take(500_000).collect::(); + let long_header = "A".repeat(500_000); format!("\ HTTP/1.1 200 OK\r\n\ {}: {}\r\n\ diff --git a/tests/server.rs b/tests/server.rs index 8561ab487c..1a3cd7f782 100644 --- a/tests/server.rs +++ b/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"); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index e7e1e8c6bd..1ec26b1204 100644 --- a/tests/support/mod.rs +++ b/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();