From 389c85bab42418a7c239b3fea13e6a2dfc70fbaf Mon Sep 17 00:00:00 2001 From: Emilie Burgun Date: Fri, 31 Jan 2025 17:26:40 +0100 Subject: [PATCH] Fix UB caused by interactions with null streams --- src/machine/lib_machine/mod.rs | 2 +- src/machine/mod.rs | 2 +- src/machine/streams.rs | 91 +++++++++++++++++++++++++++++++++- src/machine/system_calls.rs | 83 +++++++++++++++---------------- src/macros.rs | 6 --- 5 files changed, 131 insertions(+), 53 deletions(-) diff --git a/src/machine/lib_machine/mod.rs b/src/machine/lib_machine/mod.rs index 24f22fe78..35668e64e 100644 --- a/src/machine/lib_machine/mod.rs +++ b/src/machine/lib_machine/mod.rs @@ -535,7 +535,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) { 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 diff --git a/src/machine/mod.rs b/src/machine/mod.rs index a62c0caa8..0313a2d90 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -291,7 +291,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)); diff --git a/src/machine/streams.rs b/src/machine/streams.rs index d434c569a..391d4d2cd 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -982,6 +982,20 @@ fn cursor_position( } } +impl From 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)> { @@ -1626,7 +1640,7 @@ impl MachineState { match_untyped_arena_ptr!(ptr, (ArenaHeaderTag::Stream, stream) => { return if stream.is_null_stream() { - Err(self.open_permission_error(stream_as_cell!(stream), caller, arity)) + Err(self.open_permission_error(HeapCellValue::from(stream), caller, arity)) } else { Ok(stream) }; @@ -1689,7 +1703,7 @@ impl MachineState { if let Some(alias) = stream.options().get_alias() { atom_as_cell!(alias) } else { - stream_as_cell!(stream) + stream.into() }, ); @@ -1893,3 +1907,76 @@ impl MachineState { } } } + +#[cfg(test)] +mod test { + use super::*; + use crate::machine::config::*; + + #[test] + 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::>(); + + assert_eq!(results.len(), 1); + assert!(results[0].is_ok()); + } + + #[test] + fn read_null_stream() { + let mut machine = MachineBuilder::new() + .with_streams(StreamConfig::in_memory()) + .build(); + + let results = machine.run_query("get_code(C).").collect::>(); + + assert_eq!(results.len(), 1); + assert!(results[0].is_err()); + } + + #[test] + 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::>(); + + assert_eq!(results.len(), 1); + assert!(results[0].is_ok()); + } + + #[test] + 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::>(); + + assert_eq!(results.len(), 1); + assert!(results[0].is_err()); + } + + /// A variant of the [`write_null_stream`] that tries to write to a (null) input stream. + #[test] + 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::>(); + + assert_eq!(results.len(), 1); + assert!(results[0].is_err()); + } +} diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index 14f9094b8..11fe5c5f1 100644 --- a/src/machine/system_calls.rs +++ b/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( - stream_as_cell!(stream), - )); + let err = self + .machine_st + .existence_error(ExistenceError::Stream(stream.into())); return Err(self.machine_st.error_form(err, stub_gen())); } @@ -3730,7 +3730,7 @@ impl Machine { self.indices.streams = self.indices.streams.sub(&null_streams); 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(); @@ -3761,8 +3761,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); + self.machine_st.bind(var, next_stream.into()); } else { self.machine_st.fail = true; } @@ -3779,7 +3778,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 @@ -3899,7 +3898,7 @@ impl Machine { if close_result.is_err() { 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)); @@ -4472,10 +4471,9 @@ impl Machine { self.indices.streams.insert(stream); - 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; @@ -4689,7 +4687,7 @@ impl Machine { *stream.options_mut() = StreamOptions::default(); stream.options_mut().set_stream_type(StreamType::Binary); self.indices.streams.insert(stream); - let stream = stream_as_cell!(stream); + let stream: HeapCellValue = stream.into(); let handle: TypedArenaPtr = arena_alloc!(request.response, &mut self.machine_st.arena); @@ -4792,27 +4790,26 @@ impl Machine { read_heap_cell!(culprit, (HeapCellValueTag::Cons, cons_ptr) => { - match_untyped_arena_ptr!(cons_ptr, - (ArenaHeaderTag::HttpResponse, http_response) => { - let mut stream = Stream::from_http_sender( - http_response, - status_code, - headers, - &mut self.machine_st.arena + match_untyped_arena_ptr!(cons_ptr, + (ArenaHeaderTag::HttpResponse, http_response) => { + let mut stream = Stream::from_http_sender( + http_response, + status_code, + headers, + &mut self.machine_st.arena + ); + *stream.options_mut() = StreamOptions::default(); + stream.options_mut().set_stream_type(StreamType::Binary); + self.indices.streams.insert(stream); + self.machine_st.bind(stream_addr.as_var().unwrap(), stream.into()); + } + _ => { + unreachable!(); + } ); - *stream.options_mut() = StreamOptions::default(); - stream.options_mut().set_stream_type(StreamType::Binary); - self.indices.streams.insert(stream); - let stream = stream_as_cell!(stream); - self.machine_st.bind(stream_addr.as_var().unwrap(), stream); - } - _ => { - unreachable!(); - } - ); } _ => { - unreachable!(); + unreachable!(); } ); @@ -5125,7 +5122,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 @@ -6559,7 +6556,7 @@ impl Machine { self.indices.streams.insert(stream); - stream_as_cell!(stream) + HeapCellValue::from(stream) } Err(ErrorKind::PermissionDenied) => { return Err(self.machine_st.open_permission_error( @@ -6715,14 +6712,13 @@ impl Machine { self.indices.streams.insert(tcp_stream); - 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; @@ -6770,10 +6766,11 @@ impl Machine { let stream = Stream::from_tls_stream(addr, stream, &mut self.machine_st.arena); self.indices.streams.insert(stream); - 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 { @@ -6826,7 +6823,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!(); } @@ -6875,7 +6872,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)); @@ -8099,7 +8096,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] ); diff --git a/src/macros.rs b/src/macros.rs index 7778c89e6..634959304 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -218,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);