close/1 improperly handles user_input and user_output (and other stream weirdness)

[adri326]

The logic of close/1 seems to have been put together hastily:

• First, we remove the stream from machine_st.indices.streams, no matter what
• Then, if it happens to be the user_input or user_output streams, put it back
• If the stream is not Stream::Stdin, Stream::Stdout or Stream::Stderr, then remove it from machine_st.indices.stream_aliases

If user_output happens to not be Stream::Stdout (in the following test, by constructing Machine with the in_memory() preset), then current_output(S), close(S) will remove user_output from machine_st.indices.stream_aliases, but not from machine_st.indices.streams or machine_st.user_output. From then on things break downstream.

#[test]
#[cfg_attr(miri, ignore)]
fn close_memory_user_output_stream() {
    let mut machine = MachineBuilder::new()
        .with_streams(StreamConfig::in_memory())
        .build();

    let results = machine
        .run_query("current_output(Stream), close(Stream), write(hello).")
        .collect::<Vec<_>>();

    assert_eq!(results.len(), 1);
    assert!(results[0].is_err());
}

---

This is one of the rare places where IndexStore::streams differ from IndexStore::stream_aliases (the only other one being '$first_stream'/1 and '$next_stream'/1, whose behavior could be implemented in terms of stream_aliases.values()), so I'm wondering if there really is a point in having both of these, since '$next_stream'/1 expects streams to contain all non-null streams found in stream_aliases.values().

Nevermind about the thought that IndexStore::streams is not needed.

[adri326]

The example above actually runs into the issue of Term::from_heapcell not knowing how to handle streams (which from what I've heard is solved in the current iteration of rebis-dev); wrapping the code in a double negation solves that issue:

#[test]
#[cfg_attr(miri, ignore)]
fn close_memory_user_output_stream() {
    let mut machine = MachineBuilder::new()
        .with_streams(StreamConfig::in_memory())
        .build();

    let results = machine
        .run_query("\\+ \\+ (current_output(Stream), close(Stream)), write(user_output, hello).")
        .collect::<Vec<_>>();

    assert_eq!(results.len(), 1);
    assert!(results[0].is_ok());
}

write/1 is not a good indicator of the broken state that close/1 can leave the streams in, as it defaults to writing to machine.user_output. With write/2 I can force it to look up the user_output stream, which fails.

Here's the second test case that I'm using:

#[test]
#[cfg_attr(miri, ignore)]
fn close_memory_user_output_stream_twice() {
    let mut machine = MachineBuilder::new()
        .with_streams(StreamConfig::in_memory())
        .build();

    let results = machine
        .run_query("\\+ \\+ (current_output(Stream), close(Stream), close(Stream)).")
        .collect::<Vec<_>>();

    assert_eq!(results.len(), 1);
    assert!(results[0].is_ok());
}

[bakaq]

The Term::from_heapcell issue is actually solved in 975fbe4 as part of #2799. rebis-dev gets rid of parse::ast::Term, not machine::lib_machine::Term which is this one.

[adri326]

I've also unearthed another issue with '$set_stream_options'/5, which can cause a mismatch between stream_aliases and stream.options.alias. I mention this here since both this and the close/1 issue can be solved by encapsulating the logic tying together IndexStore::streams, IndexStore::stream_aliases and StreamOptions::alias.

?- open("README.md", read, S, [alias(readme)]), open(stream(S), read, _, [alias(not_readme)]), close(S).
   S = '$dropped_value'.

% From now on, we have a dead entry in `stream_aliases`, which points to a dropped stream.
% We can't read from it, since the underlying file handle was dropped, but we can access its options:

?- stream_property(readme, file_name(File)).
   File = 'README.md', unexpected.
   error(existence_error(stream,readme),get_code/2). % expected, but not found

?- get_code(readme, C).
   error(syntax_error(input_output_error),get_code/2), unexpected.
   error(existence_error(stream,readme),get_code/2). % expected, but not found

Removing the second open/4 call makes the subsequent queries behave as expected.

I'm not sure if this counts as a use-after-free, but I know for sure that it's a bad idea to read from that stream.

I also think that impl Deref for TypedArenaPtr<T> should check that self.get_tag() != ArenaHeaderTag::Dropped

[adri326]

Another one I'm now handling on my patch branch:

:- open("README.md", read, S), open(stream(S), read, _, [alias(user_output)]), close(S).

?- write(user_output, hello).
   error(existence_error(stream,user_output),write_term/0), unexpected.
   true. % Expected, but not found

[adri326]

I noticed that the example from #1070 now panics (it corrupts an OrFrame, uh oh):

?- current_output(S0), open(stream(S0), write, S, [type(binary)]).

So far re-opening a stream only modifies its properties, but I wonder if we shouldn't instead return a new, fresh stream handle, that way we don't break stuff? This would require rc-ing the streams, though, and causes headaches around stuff like mixing type(binary) and type(text).

[triska]

Creating a new stream handle for this is equally OK, or in fact even better.

In #1070, I only did the minimum that was necessary to implement the use case of outputting binary data on the terminal, and then halting.

[triska]

@adri326: An existing application where this functionality is used is Enscryerypt, to emit the decrypted data verbatim, without translation to UTF-8 encoding.

[adri326]

This last one doesn't seem to be related to close/1, so I'll open as a new issue