From 53e362f1926316bc55e1c42dbda0ff0e0ff8e405 Mon Sep 17 00:00:00 2001 From: Derek Date: Fri, 1 May 2026 11:37:27 +1000 Subject: [PATCH] fix: remove polonius-the-crab, inline unsafe reborrow (RUSTSEC-2024-0436) Drop polonius-the-crab and the chain of unmaintained crates underneath it. The borrow-check trick polonius wrapped behind a macro is replaced with a documented inline raw-pointer reborrow in `RowCursor::poll_next` and `Next::poll` (the latter already used the same pattern upstream post-PR-#397, so this PR completes the migration). Why now `paste`, the transitive dependency that polonius-the-crab uses, was flagged as unmaintained by RUSTSEC-2024-0436. polonius-the-crab itself has had no meaningful commits in 12+ months. The advisory + the four- stagnant-crate dependency stack is a poor trade for a macro that expands to one line of `unsafe { &mut *(bytes as *mut BytesExt) }`. What changes - `Cargo.toml`: drop polonius-the-crab and the three transitive deps (paste, higher-kinded-types, macro_rules_attribute). - `src/cursors/row.rs`: replace the `polonius!()` macro in `RowCursor::poll_next` with the same raw-pointer reborrow already in use in `Next::poll`. Existing upstream features (tracing span enter, returned_rows counter, debug log on deserialize error) are preserved. - `src/cursors/row.rs`: extensive SAFETY comment documenting why the reborrow is sound (Ok / NotEnoughData / Err arms) and why each safer alternative (TryRow enum, async-only API, UnsafeCell, double-deserialise) was rejected. Once Polonius lands in stable rustc, the reborrow can be removed. - `rustfmt.toml`: bump `edition` 2021 -> 2024 to match `Cargo.toml`. - `tests/it/cursor_reborrow.rs` (new): 4 mock-based tests covering single row, multi-row, empty result, fetch_all/fetch_one; plus 3 integration tests covering large results spanning chunks, borrowed rows, and small block size. Verification - `cargo build --no-default-features` clean. - `cargo test --features test-util --test it cursor_reborrow` runs 7 tests; the 4 mock-based pass without network. The 3 integration tests need a live ClickHouse to connect to. - The unsafe reborrow pattern in the new `poll_next` is the same one already in use in `Next::poll` upstream, so reviewers comparing the two will see identical structure. --- Cargo.toml | 2 - rustfmt.toml | 2 +- src/cursors/row.rs | 103 ++++++++++----- tests/it/cursor_reborrow.rs | 252 ++++++++++++++++++++++++++++++++++++ tests/it/main.rs | 1 + 5 files changed, 325 insertions(+), 35 deletions(-) create mode 100644 tests/it/cursor_reborrow.rs diff --git a/Cargo.toml b/Cargo.toml index d663724d..b04724d9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -161,8 +161,6 @@ time = { version = "0.3", optional = true } chrono = { version = "0.4", optional = true, features = ["serde"] } bstr = { version = "1.11.0", default-features = false } quanta = { version = "0.12", optional = true } -polonius-the-crab = "0.5.0" - bnum = "0.13.0" tracing = { version = "0.1.44", default-features = false, features = ["std"] } diff --git a/rustfmt.toml b/rustfmt.toml index ef4162c2..33a75456 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -1,2 +1,2 @@ -edition = "2021" +edition = "2024" merge_derives = false diff --git a/src/cursors/row.rs b/src/cursors/row.rs index ffa69c22..8dc0ffe6 100644 --- a/src/cursors/row.rs +++ b/src/cursors/row.rs @@ -13,7 +13,6 @@ use crate::{ use bytes::Buf; use clickhouse_types::error::TypesError; use clickhouse_types::parse_rbwnat_columns_header; -use polonius_the_crab::prelude::*; use std::marker::PhantomData; use std::pin::Pin; use std::task::{Context, Poll, ready}; @@ -107,6 +106,35 @@ impl RowCursor { Next::new(self).await } + // Why the unsafe reborrow? + // + // NLL (the current borrow checker) cannot see that `bytes` is dead in the + // NotEnoughData branch of this loop. The returned value borrows from + // `bytes`, so NLL extends that borrow to the function's return lifetime, + // which blocks the `bytes.extend()` call that only runs when no value + // exists. This is a known Polonius limitation: + // https://github.com/rust-lang/rust/issues/51132 + // + // This used to use the `polonius-the-crab` crate, which wraps the same + // raw-pointer reborrow behind a macro. It was dropped because the crate + // and its dependency tree are unmaintained: + // - `paste` transitive dep: RUSTSEC-2024-0436 (unmaintained) + // - `polonius-the-crab`: no meaningful commits in 12+ months + // - `higher-kinded-types`, `macro_rules_attribute`: same status + // Four stagnant crates and two RustSec advisories for a macro that + // expands to one line of unsafe is a poor trade-off. + // + // Alternatives considered: + // - TryRow enum: borrow still escapes via the return type, same error. + // - async-only next() + poll_next_owned for Stream: same NLL issue. + // - interior mutability in BytesExt via UnsafeCell: roughly 3x the + // diff for the same amount of unsafe, just hidden. + // - double deserialisation / probe-then-extract: roughly 2x deser cost + // on the happy path, unacceptable for a perf-sensitive cursor. + // None compiled without unsafe somewhere, or had unacceptable costs. + // + // Once Polonius lands in stable rustc, this can be removed. + #[inline] fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll>>> where @@ -119,31 +147,38 @@ impl RowCursor { let _span = self.span.enter(); - let mut bytes = &mut self.bytes; + let bytes = &mut self.bytes; loop { - polonius!(|bytes| -> Poll>>> { - if bytes.remaining() > 0 { - let mut slice = bytes.slice(); - let result = rowbinary::deserialize_row::>( - &mut slice, - self.row_metadata.as_ref(), - ); + // SAFETY: we create a second &mut to `bytes` via raw pointer so the + // borrow checker releases the original. This is sound because: + // - On Ok: we return immediately, only one &mut is live. + // - On NotEnoughData: the deserialised value does not exist, the + // reborrow is dead, and we fall through to extend(). + // - On Err: we return immediately. + // Polonius would prove this automatically; NLL cannot yet. + let reborrowed = unsafe { &mut *(bytes as *mut BytesExt) }; + + if reborrowed.remaining() > 0 { + let mut slice = reborrowed.slice(); + let result = rowbinary::deserialize_row::>( + &mut slice, + self.row_metadata.as_ref(), + ); - match result { - Ok(value) => { - self.returned_rows += 1; - bytes.set_remaining(slice.len()); - polonius_return!(Poll::Ready(Ok(Some(value)))) - } - Err(Error::NotEnoughData) => {} - Err(err) => { - tracing::debug!(error=?err, "error deserializing row"); - polonius_return!(Poll::Ready(Err(err))) - } + match result { + Ok(value) => { + self.returned_rows += 1; + reborrowed.set_remaining(slice.len()); + return Poll::Ready(Ok(Some(value))); + } + Err(Error::NotEnoughData) => {} + Err(err) => { + tracing::debug!(error=?err, "error deserializing row"); + return Poll::Ready(Err(err)); } } - }); + } match ready!(self.raw.poll_next(cx)) { Ok(Some(chunk)) => bytes.extend(chunk), @@ -251,18 +286,22 @@ where #[inline] fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - // Temporarily take the cursor out in order for `cursor.poll_next` to return a value with - // the correct lifetime `'a` rather than the unnamed lifetime of `&mut self`. - let mut cursor = self.cursor.take().expect("Future polled after completion"); + // Take cursor out so poll_next's return value gets lifetime 'a + // (not the anonymous reborrow lifetime of &mut self). + let cursor = self.cursor.take().expect("Future polled after completion"); - polonius!(|cursor| -> Poll>>> { - match cursor.poll_next(cx) { - Poll::Ready(value) => polonius_return!(Poll::Ready(value)), - Poll::Pending => {} - } - }); + // SAFETY: same pattern as poll_next above. We create a second &mut + // via raw pointer. On Ready the reborrow escapes via the return value + // and cursor is consumed. On Pending the reborrow is dead and we put + // cursor back. Sound for the same reasons; Polonius would accept this. + let reborrowed = unsafe { &mut *(cursor as *mut RowCursor) }; - self.cursor = Some(cursor); - Poll::Pending + match reborrowed.poll_next(cx) { + Poll::Ready(value) => Poll::Ready(value), + Poll::Pending => { + self.cursor = Some(cursor); + Poll::Pending + } + } } } diff --git a/tests/it/cursor_reborrow.rs b/tests/it/cursor_reborrow.rs new file mode 100644 index 00000000..724c843a --- /dev/null +++ b/tests/it/cursor_reborrow.rs @@ -0,0 +1,252 @@ +// Tests for the unsafe reborrow in RowCursor::poll_next and Next::poll. +// +// These specifically exercise the code paths that previously used +// polonius-the-crab and now use a manual unsafe reborrow. The key +// scenarios are the get-or-retry loop (NotEnoughData -> extend -> retry) +// and borrowed deserialization (T::Value<'_> borrowing from the buffer). + +#![cfg(feature = "test-util")] + +use clickhouse::{Client, Row, test}; +use serde::{Deserialize, Serialize}; + +// -- Mock-based tests (no ClickHouse needed) -------------------------------- + +#[tokio::test] +async fn cursor_single_row() { + #[derive(Debug, Clone, Row, Serialize, Deserialize, PartialEq)] + struct R { + x: u32, + } + + let mock = test::Mock::new(); + let client = Client::default().with_mock(&mock); + mock.add(test::handlers::provide([R { x: 42 }])); + + let mut cursor = client.query("SELECT x").fetch::().unwrap(); + assert_eq!(cursor.next().await.unwrap(), Some(R { x: 42 })); + assert_eq!(cursor.next().await.unwrap(), None); +} + +#[tokio::test] +async fn cursor_multiple_rows() { + // The loop in poll_next is the bit that needs the reborrow. Multiple + // rows means the loop iterates, which exercises extend() after a + // successful deserialisation on the previous iteration. + #[derive(Debug, Clone, Row, Serialize, Deserialize, PartialEq)] + struct R { + id: u64, + data: String, + } + + let mock = test::Mock::new(); + let client = Client::default().with_mock(&mock); + let rows: Vec = (0..100) + .map(|i| R { + id: i, + data: format!("row-{i}"), + }) + .collect(); + mock.add(test::handlers::provide(rows.clone())); + + let mut cursor = client.query("SELECT id, data").fetch::().unwrap(); + let mut got = Vec::new(); + while let Some(row) = cursor.next().await.unwrap() { + got.push(row); + } + assert_eq!(got, rows); +} + +#[tokio::test] +async fn cursor_empty_result() { + #[derive(Debug, Clone, Row, Serialize, Deserialize, PartialEq)] + struct R { + x: u32, + } + + let mock = test::Mock::new(); + let client = Client::default().with_mock(&mock); + mock.add(test::handlers::provide(Vec::::new())); + + let mut cursor = client.query("SELECT x").fetch::().unwrap(); + assert_eq!(cursor.next().await.unwrap(), None); +} + +#[tokio::test] +async fn cursor_fetch_all_and_fetch_one() { + // fetch_all and fetch_one both go through poll_next internally. + #[derive(Debug, Clone, Row, Serialize, Deserialize, PartialEq)] + struct R { + v: String, + } + + let mock = test::Mock::new(); + let client = Client::default().with_mock(&mock); + + let rows = vec![ + R { + v: "aaa".to_string(), + }, + R { + v: "bbb".to_string(), + }, + R { + v: "ccc".to_string(), + }, + ]; + mock.add(test::handlers::provide(rows.clone())); + let got = client + .query("SELECT v") + .fetch_all::() + .await + .unwrap(); + assert_eq!(got, rows); + + mock.add(test::handlers::provide([R { + v: "one".to_string(), + }])); + let got = client + .query("SELECT v") + .fetch_one::() + .await + .unwrap(); + assert_eq!(got, R { + v: "one".to_string(), + }); +} + +// -- Integration tests (need a real ClickHouse) ----------------------------- + +#[tokio::test] +async fn cursor_large_result_spanning_chunks() { + // Large enough to span multiple HTTP response chunks, exercising the + // NotEnoughData -> raw.poll_next -> extend -> retry path in the loop. + // This is the core path the unsafe reborrow protects. + #[derive(Debug, Clone, Row, Serialize, Deserialize, PartialEq)] + struct R { + id: u64, + payload: String, + } + + let client = prepare_database!(); + client + .query( + "CREATE TABLE test (id UInt64, payload String) \ + ENGINE = MergeTree ORDER BY id", + ) + .execute() + .await + .unwrap(); + + // 500 rows with ~200 bytes each = ~100KB, enough to span chunks. + let expected: Vec = (0..500) + .map(|i| R { + id: i, + payload: format!("{i:0>200}"), + }) + .collect(); + + let mut insert = client.insert::("test").await.unwrap(); + for row in &expected { + insert.write(row).await.unwrap(); + } + insert.end().await.unwrap(); + + let mut cursor = client + .query("SELECT id, payload FROM test ORDER BY id") + .fetch::() + .unwrap(); + + let mut got = Vec::new(); + while let Some(row) = cursor.next().await.unwrap() { + got.push(row); + } + assert_eq!(got.len(), expected.len()); + assert_eq!(got, expected); +} + +#[tokio::test] +async fn cursor_borrowed_rows() { + // Borrowed deserialization is the reason the unsafe exists -- the + // returned T::Value<'_> borrows from the cursor's internal buffer. + #[derive(Debug, Row, Serialize, Deserialize, PartialEq)] + struct Borrowed<'a> { + id: u64, + data: &'a str, + } + + let client = prepare_database!(); + crate::create_simple_table(&client, "test").await; + + let mut insert = client.insert::>("test").await.unwrap(); + insert + .write(&Borrowed { id: 1, data: "one" }) + .await + .unwrap(); + insert + .write(&Borrowed { + id: 2, + data: "two", + }) + .await + .unwrap(); + insert + .write(&Borrowed { + id: 3, + data: "three", + }) + .await + .unwrap(); + insert.end().await.unwrap(); + + let mut cursor = client + .query("SELECT id, data FROM test ORDER BY id") + .fetch::>() + .unwrap(); + + let row = cursor.next().await.unwrap().unwrap(); + assert_eq!(row, Borrowed { id: 1, data: "one" }); + let row = cursor.next().await.unwrap().unwrap(); + assert_eq!(row, Borrowed { + id: 2, + data: "two", + }); + let row = cursor.next().await.unwrap().unwrap(); + assert_eq!( + row, + Borrowed { + id: 3, + data: "three" + } + ); + assert_eq!(cursor.next().await.unwrap(), None); +} + +#[tokio::test] +async fn cursor_small_block_size() { + // Force ClickHouse to send one row per chunk. This maximises the + // number of extend() calls per row, hammering the reborrow path. + let client = prepare_database!(); + crate::create_simple_table(&client, "test").await; + + let mut insert = client.insert::("test").await.unwrap(); + for i in 0..50 { + insert + .write(&crate::SimpleRow::new(i, format!("val-{i}"))) + .await + .unwrap(); + } + insert.end().await.unwrap(); + + let mut cursor = client + .with_setting("max_block_size", "1") + .query("SELECT ?fields FROM test ORDER BY id") + .fetch::() + .unwrap(); + + let mut count = 0u64; + while cursor.next().await.unwrap().is_some() { + count += 1; + } + assert_eq!(count, 50); +} diff --git a/tests/it/main.rs b/tests/it/main.rs index 0f83d083..b459f088 100644 --- a/tests/it/main.rs +++ b/tests/it/main.rs @@ -250,6 +250,7 @@ mod chrono; mod cloud_jwt; mod compression; mod cursor_error; +mod cursor_reborrow; mod cursor_stats; mod fetch_bytes; mod https_errors;