Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adri326
Copy link
Contributor

@adri326 adri326 commented Jan 31, 2025

The following test triggers UB on master, while using public-facing APIs:

#[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());
}

This PR fixes #2801, by:

  • Ensuring that null streams aren't returned as null Cons (they instead return atom!("null_stream"), which is mapped to Stream::Null(Default::default()) in Machine::configure_streams().
  • Turning the stream_as_cell! macro into an impl From<Stream> for HeapCellValue.
  • Making it so that null streams can be written to (voiding the stuff written) and read from (always returning eof).

I left in the sanity checks used while debugging the issue, in case someone inadvertently creates another null Cons in the future.

…tr_as_cell!

These two functions are pretty unsafe, but having these assertions makes
it easier to catch UB in testing.
@bakaq
Copy link
Contributor

bakaq commented Jan 31, 2025

Heads up: Remember to #[cfg(miri, ignore)] tests that create a Machine. Currently they take forever and eventually fail anyway.

@Skgland
Copy link
Contributor

Skgland commented Jan 31, 2025

(This is left as a draft since I have to take a flight this evening and I'm still unsure as to what the behavior of a null stream should be. Right now they cannot be read/written from/to, and errors are thrown)

I would expect a null stream to act like reading/writing to /dev/null

@adri326 adri326 force-pushed the null-stream-safety branch from f2a4ef8 to c31e369 Compare February 2, 2025 23:08
@adri326 adri326 marked this pull request as ready for review February 2, 2025 23:08
@adri326 adri326 force-pushed the null-stream-safety branch from c31e369 to 7108e87 Compare February 2, 2025 23:12
Copy link
Contributor

@bakaq bakaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that adding a test for at_end_of_steam/1 is also a good idea.

ErrorKind::PermissionDenied,
StreamError::ReadFromOutputStream,
)),
Stream::Null(_) => Ok(buf.len()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not only an unexpected behavior for a null stream (at least if we are mimicking /dev/null behavior), but I think also plain wrong. When read, /dev/null is EOF, and that means that 0 bytes should be read (see docs). Here it means we should return Ok(0).

Returning Ok(buf.len()) means that the caller will think the buffer was filled (even though it wasn't) and use its data. buf is not necessarily zeroed before this call, and so may have garbled data from a previous use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doing anything with a null stream crashes
3 participants