fix!: Remove unsafe transmute of should_interrupt

Adds a lifetime to the ExtendedBufRead trait to specify how long the
callback provided must live.

Author: maurer

gix-protocol/src/fetch/arguments/async_io.rs

@@ -9,7 +9,7 @@ impl Arguments {
         &mut self,
         transport: &'a mut T,
         add_done_argument: bool,
-    ) -> Result<Box<dyn client::ExtendedBufRead + Unpin + 'a>, client::Error> {
+    ) -> Result<Box<dyn client::ExtendedBufRead<'a> + Unpin + 'a>, client::Error> {
         if self.haves.is_empty() {
             assert!(add_done_argument, "If there are no haves, is_done must be true.");
         }

gix-protocol/src/fetch/arguments/blocking_io.rs

@@ -10,7 +10,7 @@ impl Arguments {
         &mut self,
         transport: &'a mut T,
         add_done_argument: bool,
-    ) -> Result<Box<dyn client::ExtendedBufRead + Unpin + 'a>, client::Error> {
+    ) -> Result<Box<dyn client::ExtendedBufRead<'a> + Unpin + 'a>, client::Error> {
         if self.haves.is_empty() {
             assert!(add_done_argument, "If there are no haves, is_done must be true.");
         }

gix-protocol/src/fetch/response/async_io.rs

@@ -10,7 +10,7 @@ use crate::fetch::{
 
 async fn parse_v2_section<T>(
     line: &mut String,
-    reader: &mut (impl client::ExtendedBufRead + Unpin),
+    reader: &mut (impl client::ExtendedBufRead<'_> + Unpin),
     res: &mut Vec<T>,
     parse: impl Fn(&str) -> Result<T, response::Error>,
 ) -> Result<bool, response::Error> {
@@ -44,7 +44,7 @@ impl Response {
     /// that `git` has to use to predict how many acks are supposed to be read. We also genuinely hope that this covers it all….
     pub async fn from_line_reader(
         version: Protocol,
-        reader: &mut (impl client::ExtendedBufRead + Unpin),
+        reader: &mut (impl client::ExtendedBufRead<'_> + Unpin),
         client_expects_pack: bool,
         wants_to_negotiate: bool,
     ) -> Result<Response, response::Error> {

gix-protocol/src/fetch/response/blocking_io.rs

@@ -8,9 +8,9 @@ use crate::fetch::{
     Response,
 };
 
-fn parse_v2_section<T>(
+fn parse_v2_section<'a, T>(
     line: &mut String,
-    reader: &mut impl client::ExtendedBufRead,
+    reader: &mut impl client::ExtendedBufRead<'a>,
     res: &mut Vec<T>,
     parse: impl Fn(&str) -> Result<T, response::Error>,
 ) -> Result<bool, response::Error> {
@@ -42,9 +42,9 @@ impl Response {
     /// is to predict how to parse V1 output only, and neither `client_expects_pack` nor `wants_to_negotiate` are relevant for V2.
     /// This ugliness is in place to avoid having to resort to an [an even more complex ugliness](https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/fetch-pack.c#L583-L594)
     /// that `git` has to use to predict how many acks are supposed to be read. We also genuinely hope that this covers it all….
-    pub fn from_line_reader(
+    pub fn from_line_reader<'a>(
         version: Protocol,
-        reader: &mut impl client::ExtendedBufRead,
+        reader: &mut impl client::ExtendedBufRead<'a>,
         client_expects_pack: bool,
         wants_to_negotiate: bool,
     ) -> Result<Response, response::Error> {

gix-protocol/src/fetch_fn.rs

@@ -165,8 +165,10 @@ where
     Ok(())
 }
 
-fn setup_remote_progress<P>(progress: &mut P, reader: &mut Box<dyn gix_transport::client::ExtendedBufRead + Unpin + '_>)
-where
+fn setup_remote_progress<P>(
+    progress: &mut P,
+    reader: &mut Box<dyn gix_transport::client::ExtendedBufRead<'_> + Unpin + '_>,
+) where
     P: NestedProgress,
     P::SubProgress: 'static,
 {
@@ -176,5 +178,5 @@ where
             crate::RemoteProgress::translate_to_progress(is_err, data, &mut remote_progress);
             gix_transport::packetline::read::ProgressAction::Continue
         }
-    }) as gix_transport::client::HandleProgress));
+    }) as gix_transport::client::HandleProgress<'_>));
 }

gix-transport/src/client/async_io/bufread_ext.rs

@@ -15,7 +15,7 @@ use crate::{
 /// A function `f(is_error, text)` receiving progress or error information.
 /// As it is not a future itself, it must not block. If IO is performed within the function, be sure to spawn
 /// it onto an executor.
-pub type HandleProgress = Box<dyn FnMut(bool, &[u8]) -> ProgressAction>;
+pub type HandleProgress<'a> = Box<dyn FnMut(bool, &[u8]) -> ProgressAction + 'a>;
 
 /// This trait exists to get a version of a `gix_packetline::Provider` without type parameters,
 /// but leave support for reading lines directly without forcing them through `String`.
@@ -44,11 +44,11 @@ pub trait ReadlineBufRead: AsyncBufRead {
 
 /// Provide even more access to the underlying packet reader.
 #[async_trait(?Send)]
-pub trait ExtendedBufRead: ReadlineBufRead {
+pub trait ExtendedBufRead<'a>: ReadlineBufRead {
     /// Set the handler to which progress will be delivered.
     ///
     /// Note that this is only possible if packet lines are sent in side band mode.
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>);
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>);
     /// Peek the next data packet line. Maybe None if the next line is a packet we stop at, queryable using
     /// [`stopped_at()`][ExtendedBufRead::stopped_at()].
     async fn peek_data_line(&mut self) -> Option<io::Result<Result<&[u8], Error>>>;
@@ -70,8 +70,8 @@ impl<'a, T: ReadlineBufRead + ?Sized + 'a + Unpin> ReadlineBufRead for Box<T> {
 }
 
 #[async_trait(?Send)]
-impl<'a, T: ExtendedBufRead + ?Sized + 'a + Unpin> ExtendedBufRead for Box<T> {
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>) {
+impl<'a, T: ExtendedBufRead<'a> + ?Sized + 'a + Unpin> ExtendedBufRead<'a> for Box<T> {
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>) {
         self.deref_mut().set_progress_handler(handle_progress)
     }
 
@@ -101,7 +101,7 @@ impl<T: AsyncRead + Unpin> ReadlineBufRead
 }
 
 #[async_trait(?Send)]
-impl<'a, T: AsyncRead + Unpin> ReadlineBufRead for gix_packetline::read::WithSidebands<'a, T, HandleProgress> {
+impl<'a, T: AsyncRead + Unpin> ReadlineBufRead for gix_packetline::read::WithSidebands<'a, T, HandleProgress<'a>> {
     async fn readline(&mut self) -> Option<io::Result<Result<PacketLineRef<'_>, gix_packetline::decode::Error>>> {
         self.read_data_line().await
     }
@@ -111,8 +111,8 @@ impl<'a, T: AsyncRead + Unpin> ReadlineBufRead for gix_packetline::read::WithSid
 }
 
 #[async_trait(?Send)]
-impl<'a, T: AsyncRead + Unpin> ExtendedBufRead for gix_packetline::read::WithSidebands<'a, T, HandleProgress> {
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>) {
+impl<'a, T: AsyncRead + Unpin> ExtendedBufRead<'a> for gix_packetline::read::WithSidebands<'a, T, HandleProgress<'a>> {
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>) {
         self.set_progress_handler(handle_progress)
     }
     async fn peek_data_line(&mut self) -> Option<io::Result<Result<&[u8], Error>>> {

gix-transport/src/client/async_io/request.rs

@@ -17,7 +17,7 @@ pin_project! {
         on_into_read: MessageKind,
         #[pin]
         writer: gix_packetline::Writer<Box<dyn AsyncWrite + Unpin + 'a>>,
-        reader: Box<dyn ExtendedBufRead + Unpin + 'a>,
+        reader: Box<dyn ExtendedBufRead<'a> + Unpin + 'a>,
         trace: bool,
     }
 }
@@ -43,7 +43,7 @@ impl<'a> RequestWriter<'a> {
     /// If `trace` is true, `gix_trace` will be used on every written message or data.
     pub fn new_from_bufread<W: AsyncWrite + Unpin + 'a>(
         writer: W,
-        reader: Box<dyn ExtendedBufRead + Unpin + 'a>,
+        reader: Box<dyn ExtendedBufRead<'a> + Unpin + 'a>,
         write_mode: WriteMode,
         on_into_read: MessageKind,
         trace: bool,
@@ -102,7 +102,7 @@ impl<'a> RequestWriter<'a> {
     /// Discard the ability to write and turn this instance into the reader for obtaining the other side's response.
     ///
     /// Doing so will also write the message type this instance was initialized with.
-    pub async fn into_read(mut self) -> std::io::Result<Box<dyn ExtendedBufRead + Unpin + 'a>> {
+    pub async fn into_read(mut self) -> std::io::Result<Box<dyn ExtendedBufRead<'a> + Unpin + 'a>> {
         use futures_lite::AsyncWriteExt;
         self.write_message(self.on_into_read).await?;
         self.writer.inner_mut().flush().await?;
@@ -119,7 +119,12 @@ impl<'a> RequestWriter<'a> {
     /// It's of utmost importance to drop the request writer before reading the response as these might be inter-dependent, depending on
     /// the underlying transport mechanism. Failure to do so may result in a deadlock depending on how the write and read mechanism
     /// is implemented.
-    pub fn into_parts(self) -> (Box<dyn AsyncWrite + Unpin + 'a>, Box<dyn ExtendedBufRead + Unpin + 'a>) {
+    pub fn into_parts(
+        self,
+    ) -> (
+        Box<dyn AsyncWrite + Unpin + 'a>,
+        Box<dyn ExtendedBufRead<'a> + Unpin + 'a>,
+    ) {
         (self.writer.into_inner(), self.reader)
     }
 }

gix-transport/src/client/async_io/traits.rs

@@ -82,7 +82,7 @@ pub trait TransportV2Ext {
         capabilities: impl Iterator<Item = (&'a str, Option<impl AsRef<str>>)> + 'a,
         arguments: Option<impl Iterator<Item = bstr::BString> + 'a>,
         trace: bool,
-    ) -> Result<Box<dyn ExtendedBufRead + Unpin + '_>, Error>;
+    ) -> Result<Box<dyn ExtendedBufRead<'_> + Unpin + '_>, Error>;
 }
 
 #[async_trait(?Send)]
@@ -93,7 +93,7 @@ impl<T: Transport> TransportV2Ext for T {
         capabilities: impl Iterator<Item = (&'a str, Option<impl AsRef<str>>)> + 'a,
         arguments: Option<impl Iterator<Item = BString> + 'a>,
         trace: bool,
-    ) -> Result<Box<dyn ExtendedBufRead + Unpin + '_>, Error> {
+    ) -> Result<Box<dyn ExtendedBufRead<'_> + Unpin + '_>, Error> {
         let mut writer = self.request(WriteMode::OneLfTerminatedLinePerWriteCall, MessageKind::Flush, trace)?;
         writer.write_all(format!("command={command}").as_bytes()).await?;
         for (name, value) in capabilities {

gix-transport/src/client/blocking_io/bufread_ext.rs

@@ -10,7 +10,7 @@ use crate::{
     Protocol,
 };
 /// A function `f(is_error, text)` receiving progress or error information.
-pub type HandleProgress = Box<dyn FnMut(bool, &[u8]) -> ProgressAction>;
+pub type HandleProgress<'a> = Box<dyn FnMut(bool, &[u8]) -> ProgressAction + 'a>;
 
 /// This trait exists to get a version of a `gix_packetline::Provider` without type parameters,
 /// but leave support for reading lines directly without forcing them through `String`.
@@ -37,11 +37,11 @@ pub trait ReadlineBufRead: io::BufRead {
 }
 
 /// Provide even more access to the underlying packet reader.
-pub trait ExtendedBufRead: ReadlineBufRead {
+pub trait ExtendedBufRead<'a>: ReadlineBufRead {
     /// Set the handler to which progress will be delivered.
     ///
     /// Note that this is only possible if packet lines are sent in side band mode.
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>);
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>);
     /// Peek the next data packet line. Maybe None if the next line is a packet we stop at, queryable using
     /// [`stopped_at()`][ExtendedBufRead::stopped_at()].
     fn peek_data_line(&mut self) -> Option<io::Result<Result<&[u8], Error>>>;
@@ -61,8 +61,8 @@ impl<'a, T: ReadlineBufRead + ?Sized + 'a> ReadlineBufRead for Box<T> {
     }
 }
 
-impl<'a, T: ExtendedBufRead + ?Sized + 'a> ExtendedBufRead for Box<T> {
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>) {
+impl<'a, T: ExtendedBufRead<'a> + ?Sized + 'a> ExtendedBufRead<'a> for Box<T> {
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>) {
         self.deref_mut().set_progress_handler(handle_progress)
     }
 
@@ -89,7 +89,7 @@ impl<T: io::Read> ReadlineBufRead for gix_packetline::read::WithSidebands<'_, T,
     }
 }
 
-impl<'a, T: io::Read> ReadlineBufRead for gix_packetline::read::WithSidebands<'a, T, HandleProgress> {
+impl<'a, T: io::Read> ReadlineBufRead for gix_packetline::read::WithSidebands<'a, T, HandleProgress<'a>> {
     fn readline(&mut self) -> Option<io::Result<Result<PacketLineRef<'_>, gix_packetline::decode::Error>>> {
         self.read_data_line()
     }
@@ -99,8 +99,8 @@ impl<'a, T: io::Read> ReadlineBufRead for gix_packetline::read::WithSidebands<'a
     }
 }
 
-impl<'a, T: io::Read> ExtendedBufRead for gix_packetline::read::WithSidebands<'a, T, HandleProgress> {
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>) {
+impl<'a, T: io::Read> ExtendedBufRead<'a> for gix_packetline::read::WithSidebands<'a, T, HandleProgress<'a>> {
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>) {
         self.set_progress_handler(handle_progress)
     }
     fn peek_data_line(&mut self) -> Option<io::Result<Result<&[u8], Error>>> {

gix-transport/src/client/blocking_io/http/mod.rs

@@ -516,8 +516,8 @@ impl<H: Http, B: ReadlineBufRead + Unpin> ReadlineBufRead for HeadersThenBody<H,
     }
 }
 
-impl<H: Http, B: ExtendedBufRead + Unpin> ExtendedBufRead for HeadersThenBody<H, B> {
-    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress>) {
+impl<'a, H: Http, B: ExtendedBufRead<'a> + Unpin> ExtendedBufRead<'a> for HeadersThenBody<H, B> {
+    fn set_progress_handler(&mut self, handle_progress: Option<HandleProgress<'a>>) {
         self.body.set_progress_handler(handle_progress)
     }
 

gix-transport/src/client/blocking_io/request.rs

@@ -8,7 +8,7 @@ use crate::client::{ExtendedBufRead, MessageKind, WriteMode};
 pub struct RequestWriter<'a> {
     on_into_read: MessageKind,
     writer: gix_packetline::Writer<Box<dyn io::Write + 'a>>,
-    reader: Box<dyn ExtendedBufRead + Unpin + 'a>,
+    reader: Box<dyn ExtendedBufRead<'a> + Unpin + 'a>,
     trace: bool,
 }
 
@@ -35,7 +35,7 @@ impl<'a> RequestWriter<'a> {
     /// If `trace` is true, `gix_trace` will be used on every written message or data.
     pub fn new_from_bufread<W: io::Write + 'a>(
         writer: W,
-        reader: Box<dyn ExtendedBufRead + Unpin + 'a>,
+        reader: Box<dyn ExtendedBufRead<'a> + Unpin + 'a>,
         write_mode: WriteMode,
         on_into_read: MessageKind,
         trace: bool,
@@ -89,7 +89,7 @@ impl<'a> RequestWriter<'a> {
     /// Discard the ability to write and turn this instance into the reader for obtaining the other side's response.
     ///
     /// Doing so will also write the message type this instance was initialized with.
-    pub fn into_read(mut self) -> std::io::Result<Box<dyn ExtendedBufRead + Unpin + 'a>> {
+    pub fn into_read(mut self) -> std::io::Result<Box<dyn ExtendedBufRead<'a> + Unpin + 'a>> {
         self.write_message(self.on_into_read)?;
         self.writer.inner_mut().flush()?;
         Ok(self.reader)
@@ -105,7 +105,7 @@ impl<'a> RequestWriter<'a> {
     /// It's of utmost importance to drop the request writer before reading the response as these might be inter-dependent, depending on
     /// the underlying transport mechanism. Failure to do so may result in a deadlock depending on how the write and read mechanism
     /// is implemented.
-    pub fn into_parts(self) -> (Box<dyn io::Write + 'a>, Box<dyn ExtendedBufRead + Unpin + 'a>) {
+    pub fn into_parts(self) -> (Box<dyn io::Write + 'a>, Box<dyn ExtendedBufRead<'a> + Unpin + 'a>) {
         (self.writer.into_inner(), self.reader)
     }
 }

gix-transport/src/client/blocking_io/traits.rs

@@ -75,7 +75,7 @@ pub trait TransportV2Ext {
         capabilities: impl Iterator<Item = (&'a str, Option<impl AsRef<str>>)> + 'a,
         arguments: Option<impl Iterator<Item = bstr::BString>>,
         trace: bool,
-    ) -> Result<Box<dyn ExtendedBufRead + Unpin + '_>, Error>;
+    ) -> Result<Box<dyn ExtendedBufRead<'_> + Unpin + '_>, Error>;
 }
 
 impl<T: Transport> TransportV2Ext for T {
@@ -85,7 +85,7 @@ impl<T: Transport> TransportV2Ext for T {
         capabilities: impl Iterator<Item = (&'a str, Option<impl AsRef<str>>)> + 'a,
         arguments: Option<impl Iterator<Item = BString>>,
         trace: bool,
-    ) -> Result<Box<dyn ExtendedBufRead + Unpin + '_>, Error> {
+    ) -> Result<Box<dyn ExtendedBufRead<'_> + Unpin + '_>, Error> {
         let mut writer = self.request(WriteMode::OneLfTerminatedLinePerWriteCall, MessageKind::Flush, trace)?;
         writer.write_all(format!("command={command}").as_bytes())?;
         for (name, value) in capabilities {

gix/src/remote/connection/fetch/receive_pack.rs

@@ -410,24 +410,14 @@ fn add_shallow_args(
     Ok((shallow_commits, shallow_lock))
 }
 
-fn setup_remote_progress(
+fn setup_remote_progress<'a>(
     progress: &mut dyn crate::DynNestedProgress,
-    reader: &mut Box<dyn gix_protocol::transport::client::ExtendedBufRead + Unpin + '_>,
-    should_interrupt: &AtomicBool,
+    reader: &mut Box<dyn gix_protocol::transport::client::ExtendedBufRead<'a> + Unpin + 'a>,
+    should_interrupt: &'a AtomicBool,
 ) {
     use gix_protocol::transport::client::ExtendedBufRead;
     reader.set_progress_handler(Some(Box::new({
         let mut remote_progress = progress.add_child_with_id("remote".to_string(), ProgressId::RemoteProgress.into());
-        // SAFETY: Ugh, so, with current Rust I can't declare lifetimes in the involved traits the way they need to
-        //         be and I also can't use scoped threads to pump from local scopes to an Arc version that could be
-        //         used here due to the this being called from sync AND async code (and the async version doesn't work
-        //         with a surrounding `std::thread::scope()`.
-        //         Thus there is only claiming this is 'static which we know works for *our* implementations of ExtendedBufRead
-        //         and typical implementations, but of course it's possible for user code to come along and actually move this
-        //         handler into a context where it can outlive the current function. Is this going to happen? Probably not unless
-        //         somebody really wants to break it. So, with standard usage this value is never used past its actual lifetime.
-        #[allow(unsafe_code)]
-        let should_interrupt: &'static AtomicBool = unsafe { std::mem::transmute(should_interrupt) };
         move |is_err: bool, data: &[u8]| {
             gix_protocol::RemoteProgress::translate_to_progress(is_err, data, &mut remote_progress);
             if should_interrupt.load(Ordering::Relaxed) {
@@ -436,5 +426,5 @@ fn setup_remote_progress(
                 ProgressAction::Continue
             }
         }
-    }) as gix_protocol::transport::client::HandleProgress));
+    }) as gix_protocol::transport::client::HandleProgress<'a>));
 }