Merge pull request #2802 from adri326/null-stream-safety

Fix UB when interacting with Stream::Null(_)

src/ffi.rs

@@ -89,7 +89,6 @@ impl ForeignFunctionTable {
     }
 
     fn map_type_ffi(&mut self, source: &Atom) -> *mut ffi_type {
-        unsafe {
         match source {
             atom!("sint64") => addr_of_mut!(types::sint64),
             atom!("sint32") => addr_of_mut!(types::sint32),
@@ -111,7 +110,6 @@ impl ForeignFunctionTable {
             },
         }
     }
-    }
 
     pub(crate) fn load_library(
         &mut self,

src/lib/builtins.pl

@@ -2188,12 +2188,10 @@ stream_property(S, P) :-
 %% at_end_of_stream(+Stream).
 %
 % True iff the stream Stream has ended
-at_end_of_stream(S_or_a) :-
+at_end_of_stream(S) :-
-    (  var(S_or_a) ->
+    (  var(S) ->
        throw(error(instantiation_error, at_end_of_stream/1))
-    ;  atom(S_or_a) ->
+    ;  true
-       stream_property(S, alias(S_or_a))
-    ;  S = S_or_a
     ),
     stream_property(S, end_of_stream(E)),
     ( E = at -> true ; E = past ).
@@ -2205,7 +2203,7 @@ at_end_of_stream :-
     current_input(S),
     stream_property(S, end_of_stream(E)),
     !,
-    ( E = at ; E = past ).
+    ( E = at -> true ; E = past ).
 
 %% set_stream_position(+Stream, +Position).
 %

src/machine/lib_machine/mod.rs

@@ -559,7 +559,7 @@ impl Machine {
     /// Consults a module into the [`Machine`] from a string.
     pub fn consult_module_string(&mut self, module_name: &str, program: impl Into<String>) {
         let stream = Stream::from_owned_string(program.into(), &mut self.machine_st.arena);
-        self.machine_st.registers[1] = stream_as_cell!(stream);
+        self.machine_st.registers[1] = stream.into();
         self.machine_st.registers[2] = atom_as_cell!(&atom_table::AtomTable::build_with(
             &self.machine_st.atom_tbl,
             module_name

src/machine/mod.rs

@@ -298,7 +298,7 @@ impl Machine {
     }
 
     fn load_file(&mut self, path: &str, stream: Stream) {
-        self.machine_st.registers[1] = stream_as_cell!(stream);
+        self.machine_st.registers[1] = stream.into();
         self.machine_st.registers[2] =
             atom_as_cell!(AtomTable::build_with(&self.machine_st.atom_tbl, path));
 
@@ -500,6 +500,12 @@ impl Machine {
             .set_stream(atom!("user_output"), self.user_output);
         self.indices
             .set_stream(atom!("user_error"), self.user_error);
+
+        let mut null_options = StreamOptions::default();
+        null_options.set_alias_to_atom_opt(Some(atom!("null_stream")));
+
+        self.indices
+            .set_stream(atom!("null_stream"), Stream::Null(null_options));
     }
 
     #[inline(always)]

src/machine/streams.rs

@@ -967,11 +967,11 @@ impl Read for Stream {
             Stream::OutputFile(_)
             | Stream::StandardError(_)
             | Stream::StandardOutput(_)
-            | Stream::Null(_)
             | Stream::Callback(_) => Err(std::io::Error::new(
                 ErrorKind::PermissionDenied,
                 StreamError::ReadFromOutputStream,
             )),
+            Stream::Null(_) => Ok(0),
         }
     }
 }
@@ -994,11 +994,11 @@ impl Write for Stream {
                 ErrorKind::PermissionDenied,
                 StreamError::WriteToInputStream,
             )),
+            Stream::Null(_) => Ok(buf.len()),
             Stream::StaticString(_)
             | Stream::InputChannel(_)
             | Stream::Readline(_)
-            | Stream::InputFile(..)
+            | Stream::InputFile(..) => Err(std::io::Error::new(
-            | Stream::Null(_) => Err(std::io::Error::new(
                 ErrorKind::PermissionDenied,
                 StreamError::WriteToInputStream,
             )),
@@ -1022,11 +1022,11 @@ impl Write for Stream {
                 ErrorKind::PermissionDenied,
                 StreamError::FlushToInputStream,
             )),
+            Stream::Null(_) => Ok(()),
             Stream::StaticString(_)
             | Stream::InputChannel(_)
             | Stream::Readline(_)
-            | Stream::InputFile(_)
+            | Stream::InputFile(_) => Err(std::io::Error::new(
-            | Stream::Null(_) => Err(std::io::Error::new(
                 ErrorKind::PermissionDenied,
                 StreamError::FlushToInputStream,
             )),
@@ -1115,6 +1115,20 @@ fn cursor_position<T>(
     }
 }
 
+impl From<Stream> for HeapCellValue {
+    #[inline(always)]
+    fn from(stream: Stream) -> Self {
+        if stream.is_null_stream() {
+            let res = atom!("null_stream");
+            atom_as_cell!(res)
+        } else {
+            let res = stream.as_ptr();
+            debug_assert!(!res.is_null());
+            raw_ptr_as_cell!(res)
+        }
+    }
+}
+
 impl Stream {
     #[inline]
     pub(crate) fn position(&mut self) -> Option<(u64, usize)> {
@@ -1267,6 +1281,7 @@ impl Stream {
                     }
                 }
             }
+            Stream::Null(_) => AtEndOfStream::At,
             #[cfg(feature = "http")]
             Stream::HttpRead(stream_layout) => {
                 if stream_layout
@@ -1522,7 +1537,8 @@ impl Stream {
             | Stream::InputChannel(_)
             | Stream::Readline(_)
             | Stream::StaticString(_)
-            | Stream::InputFile(..) => true,
+            | Stream::InputFile(..)
+            | Stream::Null(_) => true,
             _ => false,
         }
     }
@@ -1538,8 +1554,9 @@ impl Stream {
             | Stream::StandardOutput(_)
             | Stream::NamedTcp(..)
             | Stream::Byte(_)
+            | Stream::OutputFile(..)
             | Stream::Callback(_)
-            | Stream::OutputFile(..) => true,
+            | Stream::Null(_) => true,
             _ => false,
         }
     }
@@ -1795,7 +1812,7 @@ impl MachineState {
                             debug_assert_eq!(arity, 0);
 
                             return match indices.get_stream(name) {
-                                Some(stream) if !stream.is_null_stream() => Ok(stream),
+                                Some(stream) => Ok(stream),
                                 _ => {
                                     let stub = functor_stub(caller, arity);
                                     let addr = atom_as_cell!(name);
@@ -1813,7 +1830,7 @@ impl MachineState {
                             debug_assert_eq!(arity, 0);
 
                             return match indices.get_stream(name) {
-                                Some(stream) if !stream.is_null_stream() => Ok(stream),
+                                Some(stream) => Ok(stream),
                                 _ => {
                                     let stub = functor_stub(caller, arity);
                                     let addr = atom_as_cell!(name);
@@ -1827,11 +1844,10 @@ impl MachineState {
                         (HeapCellValueTag::Cons, ptr) => {
                             match_untyped_arena_ptr!(ptr,
                                 (ArenaHeaderTag::Stream, stream) => {
-                                    return if stream.is_null_stream() {
+                                    if stream.is_null_stream() {
-                                        Err(self.open_permission_error(stream_as_cell!(stream), caller, arity))
+                                        unreachable!("Null streams have no Cons representation");
-                                    } else {
+                                    }
-                                        Ok(stream)
+                                    return Ok(stream);
-                                    };
                                 }
                                 (ArenaHeaderTag::Dropped, _value) => {
                                     let stub = functor_stub(caller, arity);
@@ -1891,7 +1907,7 @@ impl MachineState {
             if let Some(alias) = stream.options().get_alias() {
                 atom_as_cell!(alias)
             } else {
-                stream_as_cell!(stream)
+                stream.into()
             },
         );
 
@@ -2097,11 +2113,16 @@ impl MachineState {
 }
 
 #[cfg(test)]
-mod tests {
+mod test {
     use crate::*;
     use std::{cell::RefCell, io::Read, io::Write, rc::Rc};
 
-    fn succeeded(answer: Vec<Result<LeafAnswer, Term>>) -> bool {
+    use crate::machine::config::*;
+    use crate::LeafAnswer;
+
+    use super::{Stream, StreamOptions};
+
+    fn succeeded<T>(answer: Vec<Result<LeafAnswer, T>>) -> bool {
         // Ideally this should be a method in QueryState or LeafAnswer.
         matches!(
             answer[0].as_ref(),
@@ -2109,6 +2130,13 @@ mod tests {
         )
     }
 
+    fn is_successful<T>(answer: &Result<LeafAnswer, T>) -> bool {
+        matches!(
+            answer,
+            Ok(LeafAnswer::True) | Ok(LeafAnswer::LeafAnswer { .. })
+        )
+    }
+
     #[test]
     #[cfg_attr(miri, ignore)]
     fn user_input_string_stream() {
@@ -2248,6 +2276,19 @@ mod tests {
         assert_eq!(actual, "hello");
     }
 
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn current_input_null_stream() {
+        let mut machine = MachineBuilder::new()
+            .with_streams(StreamConfig::in_memory())
+            .build();
+
+        let results = machine.run_query("current_input(S).").collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(is_successful(&results[0]));
+    }
+
     #[test]
     #[cfg_attr(miri, ignore)]
     fn close_memory_user_output_stream_twice() {
@@ -2263,6 +2304,23 @@ mod tests {
         assert!(results[0].is_ok());
     }
 
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn read_null_stream() {
+        let mut machine = MachineBuilder::new()
+            .with_streams(StreamConfig::in_memory())
+            .build();
+
+        let results = machine.run_query("get_code(C).").collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(
+            is_successful(&results[0]),
+            "Expected read to succeed, got {:?}",
+            results[0]
+        );
+    }
+
     #[test]
     #[cfg_attr(miri, ignore)]
     fn close_realiased_stream() {
@@ -2285,6 +2343,21 @@ mod tests {
         assert!(results[0].is_ok());
     }
 
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn current_output_null_stream() {
+        // TODO: switch to a proper solution for configuring the machine with null streams
+        // once `StreamConfig` supports it.
+        let mut machine = MachineBuilder::new().build();
+        machine.user_output = Stream::Null(StreamOptions::default());
+        machine.configure_streams();
+
+        let results = machine.run_query("current_output(S).").collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(is_successful(&results[0]));
+    }
+
     #[test]
     #[cfg_attr(miri, ignore)]
     fn close_realiased_user_output() {
@@ -2308,4 +2381,100 @@ mod tests {
         assert_eq!(results.len(), 1);
         assert!(results[0].is_ok());
     }
+
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn write_null_stream() {
+        // TODO: switch to a proper solution for configuring the machine with null streams
+        // once `StreamConfig` supports it.
+        let mut machine = MachineBuilder::new().build();
+        machine.user_output = Stream::Null(StreamOptions::default());
+        machine.configure_streams();
+
+        let results = machine.run_query("write(hello).").collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(
+            is_successful(&results[0]),
+            "Expected write to succeed, got {:?}",
+            results[0]
+        );
+    }
+
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn put_code_null_stream() {
+        // TODO: switch to a proper solution for configuring the machine with null streams
+        // once `StreamConfig` supports it.
+        let mut machine = MachineBuilder::new().build();
+        machine.user_output = Stream::Null(StreamOptions::default());
+        machine.configure_streams();
+
+        let results = machine
+            .run_query("put_code(user_output, 65).")
+            .collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(
+            is_successful(&results[0]),
+            "Expected write to succeed, got {:?}",
+            results[0]
+        );
+    }
+
+    /// A variant of the [`write_null_stream`] that tries to write to a (null) input stream.
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn write_null_input_stream() {
+        let mut machine = MachineBuilder::new()
+            .with_streams(StreamConfig::in_memory())
+            .build();
+
+        let results = machine
+            .run_query("current_input(Stream), write(Stream, hello).")
+            .collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(
+            is_successful(&results[0]),
+            "Expected write to succeed, got {:?}",
+            results[0]
+        );
+    }
+
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn at_end_of_stream_0_null_stream() {
+        let mut machine = MachineBuilder::new()
+            .with_streams(StreamConfig::in_memory())
+            .build();
+
+        let results = machine.run_query("at_end_of_stream.").collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(
+            is_successful(&results[0]),
+            "Expected at_end_of_stream to succeed, got {:?}",
+            results[0]
+        );
+    }
+
+    #[test]
+    #[cfg_attr(miri, ignore)]
+    fn at_end_of_stream_1_null_stream() {
+        let mut machine = MachineBuilder::new()
+            .with_streams(StreamConfig::in_memory())
+            .build();
+
+        let results = machine
+            .run_query("current_input(Stream), at_end_of_stream(Stream).")
+            .collect::<Vec<_>>();
+
+        assert_eq!(results.len(), 1);
+        assert!(
+            is_successful(&results[0]),
+            "Expected at_end_of_stream to succeed, got {:?}",
+            results[0]
+        );
+    }
 }

src/machine/system_calls.rs

@@ -1881,7 +1881,7 @@ impl Machine {
         let stream = self.user_input;
 
         if let Some(var) = addr.as_var() {
-            self.machine_st.bind(var, stream_as_cell!(stream));
+            self.machine_st.bind(var, stream.into());
             return Ok(());
         }
 
@@ -1916,7 +1916,7 @@ impl Machine {
         let stream = self.user_output;
 
         if let Some(var) = addr.as_var() {
-            self.machine_st.bind(var, stream_as_cell!(stream));
+            self.machine_st.bind(var, stream.into());
             return Ok(());
         }
 
@@ -3273,7 +3273,7 @@ impl Machine {
             match stream.write_all(&bytes) {
                 Ok(_) => {}
                 _ => {
-                    let addr = stream_as_cell!(stream);
+                    let addr = stream.into();
                     let err = self
                         .machine_st
                         .existence_error(ExistenceError::Stream(addr));
@@ -3323,7 +3323,7 @@ impl Machine {
                         _ => {
                             let err = self
                                 .machine_st
-                                .existence_error(ExistenceError::Stream(stream_as_cell!(stream)));
+                                .existence_error(ExistenceError::Stream(stream.into()));
 
                             return Err(self.machine_st.error_form(err, stub_gen()));
                         }
@@ -3336,9 +3336,9 @@ impl Machine {
                                 return Ok(());
                             }
                             _ => {
-                                let err = self.machine_st.existence_error(ExistenceError::Stream(
+                                let err = self
-                                    stream_as_cell!(stream),
+                                    .machine_st
-                                ));
+                                    .existence_error(ExistenceError::Stream(stream.into()));
 
                                 return Err(self.machine_st.error_form(err, stub_gen()));
                             }
@@ -3722,7 +3722,7 @@ impl Machine {
             .next();
 
         if let Some(first_stream) = first_stream {
-            let stream = stream_as_cell!(first_stream);
+            let stream = first_stream.into();
 
             let var = self.deref_register(1).as_var().unwrap();
 
@@ -3745,8 +3745,7 @@ impl Machine {
         if let Some(next_stream) = next_stream {
             let var = self.deref_register(2).as_var().unwrap();
 
-            let next_stream = stream_as_cell!(next_stream);
+            self.machine_st.bind(var, next_stream.into());
-            self.machine_st.bind(var, next_stream);
         } else {
             self.machine_st.fail = true;
         }
@@ -3763,7 +3762,7 @@ impl Machine {
 
         if !stream.is_output_stream() {
             let stub = functor_stub(atom!("flush_output"), 1);
-            let addr = stream_as_cell!(stream);
+            let addr = HeapCellValue::from(stream);
 
             let err =
                 self.machine_st
@@ -3859,14 +3858,14 @@ impl Machine {
 
         self.indices.remove_stream(stream);
 
-        stream.close().map_err(|_| {
+        stream.close().or_else(|_| {
             let stub = functor_stub(atom!("close"), 1);
-            let addr = stream_as_cell!(stream);
+            let addr = stream.into();
             let err = self
                 .machine_st
                 .existence_error(ExistenceError::Stream(addr));
 
-            self.machine_st.error_form(err, stub)
+            Err(self.machine_st.error_form(err, stub))
         })
     }
 
@@ -4434,10 +4433,9 @@ impl Machine {
                             .map_err(|stub_gen| stub_gen(&mut self.machine_st))
                             .unwrap();
 
-                        let stream = stream_as_cell!(stream);
-
                         let stream_addr = self.deref_register(2);
-                        self.machine_st.bind(stream_addr.as_var().unwrap(), stream);
+                        self.machine_st
+                            .bind(stream_addr.as_var().unwrap(), stream.into());
                     }
                     Err(_) => {
                         self.machine_st.fail = true;
@@ -4655,7 +4653,7 @@ impl Machine {
                     self.indices.add_stream(stream, atom!("http_accept"), 7)
                         .map_err(|stub_gen| stub_gen(&mut self.machine_st))?;
 
-                let stream = stream_as_cell!(stream);
+                    let stream: HeapCellValue = stream.into();
 
                     let handle: TypedArenaPtr<HttpResponse> = arena_alloc!(request.response, &mut self.machine_st.arena);
 
@@ -4766,15 +4764,13 @@ impl Machine {
                             headers,
                             &mut self.machine_st.arena
                         );
+
                         *stream.options_mut() = StreamOptions::default();
                         stream.options_mut().set_stream_type(StreamType::Binary);
 
-
                         self.indices.add_stream(stream, atom!("http_answer"), 4)
                             .map_err(|stub_gen| stub_gen(&mut self.machine_st))?;
-
+                        self.machine_st.bind(stream_addr.as_var().unwrap(), stream.into());
-                let stream = stream_as_cell!(stream);
-                self.machine_st.bind(stream_addr.as_var().unwrap(), stream);
                     }
                     _ => {
                         unreachable!();
@@ -5094,7 +5090,7 @@ impl Machine {
 
             let stream_var = self.deref_register(3);
             self.machine_st
-                .bind(stream_var.as_var().unwrap(), stream_as_cell!(stream));
+                .bind(stream_var.as_var().unwrap(), stream.into());
         } else {
             let err = self
                 .machine_st
@@ -6524,7 +6520,7 @@ impl Machine {
                     .add_stream(stream, atom!("socket_client_open"), 7)
                     .map_err(|stub_gen| stub_gen(&mut self.machine_st))?;
 
-                stream_as_cell!(stream)
+                HeapCellValue::from(stream)
             }
             Err(ErrorKind::PermissionDenied) => {
                 return Err(self.machine_st.open_permission_error(
@@ -6679,14 +6675,13 @@ impl Machine {
                                         stub_gen(&mut self.machine_st)
                                     })?;
 
-                                 let tcp_stream = stream_as_cell!(tcp_stream);
                                  let client = atom_as_cell!(client);
 
                                  let client_addr = self.deref_register(2);
                                  let stream_addr = self.deref_register(3);
 
                                  self.machine_st.bind(client_addr.as_var().unwrap(), client);
-                                 self.machine_st.bind(stream_addr.as_var().unwrap(), tcp_stream);
+                                 self.machine_st.bind(stream_addr.as_var().unwrap(), tcp_stream.into());
                              }
                              None => {
                                  self.machine_st.fail = true;
@@ -6737,10 +6732,11 @@ impl Machine {
                 .add_stream(stream, atom!("tls_client_negotiate"), 3)
                 .map_err(|stub_gen| stub_gen(&mut self.machine_st))?;
 
-            self.machine_st.heap.push(stream_as_cell!(stream));
+            // FIXME: why are we pushing a random, unreferenced cell on the heap?
+            self.machine_st.heap.push(stream.into());
             let stream_addr = self.deref_register(3);
             self.machine_st
-                .bind(stream_addr.as_var().unwrap(), stream_as_cell!(stream));
+                .bind(stream_addr.as_var().unwrap(), stream.into());
 
             Ok(())
         } else {
@@ -6796,7 +6792,7 @@ impl Machine {
 
             let stream_addr = self.deref_register(4);
             self.machine_st
-                .bind(stream_addr.as_var().unwrap(), stream_as_cell!(stream));
+                .bind(stream_addr.as_var().unwrap(), stream.into());
         } else {
             unreachable!();
         }
@@ -6845,7 +6841,7 @@ impl Machine {
             let err = self.machine_st.permission_error(
                 Permission::Reposition,
                 atom!("stream"),
-                stream_as_cell!(stream),
+                HeapCellValue::from(stream),
             );
 
             return Err(self.machine_st.error_form(err, stub));
@@ -8069,7 +8065,7 @@ impl Machine {
                 let lib_stream = Stream::from_static_string(library, &mut self.machine_st.arena);
                 unify!(
                     self.machine_st,
-                    stream_as_cell!(lib_stream),
+                    HeapCellValue::from(lib_stream),
                     self.machine_st.registers[2]
                 );
 

src/macros.rs

@@ -175,6 +175,7 @@ macro_rules! raw_ptr_as_cell {
         // TODO use <*{const,mut} _>::addr instead of as when the strict_provenance feature is stable rust-lang/rust#95228
         // we might need <*{const,mut} _>::expose_provenance for strict provenance, dependening on how we recreate a pointer later
         let ptr : *const _ = $ptr;
+        debug_assert!(!$ptr.is_null());
         HeapCellValue::from_ptr_addr(ptr as usize)
     }};
 }
@@ -217,12 +218,6 @@ macro_rules! string_as_pstr_cell {
     }};
 }
 
-macro_rules! stream_as_cell {
-    ($ptr:expr) => {
-        raw_ptr_as_cell!($ptr.as_ptr())
-    };
-}
-
 macro_rules! cell_as_stream {
     ($cell:expr) => {{
         let ptr = cell_as_untyped_arena_ptr!($cell);

src/types.rs

@@ -656,6 +656,7 @@ pub struct UntypedArenaPtr {
 impl UntypedArenaPtr {
     #[inline(always)]
     pub fn build_with(ptr: usize) -> Self {
+        debug_assert!(ptr != 0);
         UntypedArenaPtr::new().with_ptr(ptr as u64)
     }
 }
@@ -698,6 +699,7 @@ impl UntypedArenaPtr {
     #[inline]
     pub fn get_tag(self) -> ArenaHeaderTag {
         unsafe {
+            debug_assert!(!self.get_ptr().is_null());
             let header = *(self.get_ptr() as *const ArenaHeader);
             header.get_tag()
         }