Skip to content

Commit

Permalink
Fix UB caused by interactions with null streams
Browse files Browse the repository at this point in the history
  • Loading branch information
@adri326
adri326 committed Jan 31, 2025
1 parent 079a693 commit 389c85b
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 53 deletions.
2 changes: 1 addition & 1 deletion src/machine/lib_machine/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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
Expand Down
2 changes: 1 addition & 1 deletion src/machine/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
91 changes: 89 additions & 2 deletions src/machine/streams.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,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)> {
Expand Down Expand Up @@ -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)
};
Expand Down Expand Up @@ -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()
},
);

Expand Down Expand Up @@ -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::<Vec<_>>();

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::<Vec<_>>();

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::<Vec<_>>();

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::<Vec<_>>();

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::<Vec<_>>();

assert_eq!(results.len(), 1);
assert!(results[0].is_err());
}
}
83 changes: 40 additions & 43 deletions src/machine/system_calls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(());
}

Expand Down Expand Up @@ -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(());
}

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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()));
}
Expand All @@ -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()));
}
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
}
Expand All @@ -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
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<HttpResponse> = arena_alloc!(request.response, &mut self.machine_st.arena);

Expand Down Expand Up @@ -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!();
}
);

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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!();
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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]
);

Expand Down
6 changes: 0 additions & 6 deletions src/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 389c85b

Please sign in to comment.