Re: [pve-devel] [PATCH xtermjs] termproxy: rewrite in rust

Re: [pve-devel] [PATCH xtermjs] termproxy: rewrite in rust

[Wolfgang Bumiller]

Some comments.

On Mon, Jul 06, 2020 at 12:57:21PM +0200, Dominik Csapak wrote:
> termproxy is now completely written in rust (instead of perl) but
> it is a drop-in replacement
(...)
> diff --git a/src/main.rs b/src/main.rs
> new file mode 100644
> index 0000000..459a3c0
> --- /dev/null
> +++ b/src/main.rs
> @@ -0,0 +1,378 @@
(...)

> +use std::io::{Error, ErrorKind, Result, Write};

None of our code currently imports a fixed `Result` type, please prefer
just typing `io::Result` (it's not that much longer).

(...)
> +
> +fn remove_number(buf: &mut ByteBuffer) -> Option<usize> {
> +    loop {
> +        let data = buf.get_data_slice();
> +
> +        if let Some(pos) = data.iter().position(|&x| x == ':' as u8) {

use b':' (see `cargo +nightly clippy`)

> +            let len = buf.consume(pos);

please don't use the name `len` for a `Box<[u8]>`, that's confusing

> +            buf.consume(1); // the ':'

reminder: this `consume` currently returns a heap-allocated copy of the
consumed data (this should be changed in the ByteBuffer API)

> +            let len = match String::from_utf8_lossy(&len).parse() {

`from_utf8_lossy` potentially allocates, but whenever it does, `parse()`
will always fail, so consider using `std::str::from_utf8` instead...

> +                Ok(len) => len,
> +                Err(err) => {
> +                    eprintln!("error parsing number: '{}'", err);
> +                    break;
> +                }
> +            };
> +            return Some(len);
> +        } else if data.len() > 20 {
> +            buf.consume(20);
> +        } else {
> +            break;
> +        }
> +    }
> +    None
> +}
> +
> +fn process_queue(buf: &mut ByteBuffer, pty: &mut PTY) -> Option<usize> {
> +    if buf.is_empty() {
> +        return None;
> +    }
> +
> +    loop {
> +        let data = buf.get_data_slice();
> +        let len = buf.data_size();

`len` is used only once here so maybe just drop it

> +
> +        if len < 2 {

and then `data.len()` will be shorter and easier to follow than `buf.data_size()`

> +            break;
> +        }
> +
> +        let msgtype = data[0] - '0' as u8;

b'0' again

> +
> +        if msgtype == 0 {

please introduce some `const`s with meaningful names for message types

> +            buf.consume(2);
> +            if let Some(len) = remove_number(buf) {
> +                return Some(len);
> +            }
> +        } else if msgtype == 1 {
> +            buf.consume(2);
> +            if let Some(cols) = remove_number(buf) {
> +                if let Some(rows) = remove_number(buf) {
> +                    pty.set_size(cols as u16, rows as u16).ok()?;
> +                }

Shouldn't the inner `if` have an `else`? `cols` was parsed successfully
here, so if there's no `rows` we'll re-loop back to grabbing the message
type, simply discarding the `cols` value?

> +            }
> +        // ignore incomplete messages
> +        } else if msgtype == 2 {
> +            buf.consume(1);
> +            // ignore ping
> +        } else {
> +            buf.consume(1);
> +            // ignore invalid
> +        }
> +    }
> +
> +    None
> +}
> +
> +/// Reads from the stream and returns the first line and the rest
> +fn read_ticket_line(
> +    stream: &mut TcpStream,
> +    buf: &mut ByteBuffer,
> +    timeout: u64,

`u64` is a really bad type for a timeout, it doesn't say anything about
the unit, also, the implementation isn't necessarily using time to
measure this (see below).

> +) -> Result<(Box<[u8]>, Box<[u8]>)> {

Clippy complains, but won't complaina bout a pair of `Vec<u8>` which you
already have anyway (but not really, see below...)

> +    stream.set_read_timeout(Some(Duration::new(1, 0)))?;

This makes the reader believe that `timeout` will be in seconds but ...

> +    let mut count = 0;
> +    while !buf.get_data_slice().contains(&('\n' as u8)) {
> +        if buf.is_full() || count >= timeout {
> +            return Err(Error::new(
> +                ErrorKind::InvalidData,
> +                "authentication data is invalid",
> +            ));
> +        }
> +        buf.read_from(stream)?;
> +        count += 1;

... consider a short burst of 1-byte packets arriving, as many as the
number in `timeout`, then we also reach the count.

> +    }
> +
> +    stream.set_read_timeout(None)?;
> +    let newline_idx = buf
> +        .get_data_slice()
> +        .iter()
> +        .position(|&x| x == '\n' as u8)
> +        .unwrap();
> +
> +    let line = buf.consume(newline_idx);
> +    buf.consume(1); // discard newline
> +
> +    let mut iter = line.splitn(2, |&c| c == ':' as u8);
> +    let username = iter.next().unwrap();
> +    let ticket = iter.next().unwrap();

These `unwrap()`s are invalid. Consider using `.position()` to find the
b':' to have only 1 error case, then use the slice's `split_at()` method
to get a &[u8] 2-tuple:

    match line.iter().position(|&b| b == b':') {
        Some(pos) => {
            let (username, ticket) = line.split_at(pos);
            Ok((username.into(), ticket.into()))
        }
        None => bail!("with a nice error message"),
    }

> +    Ok((
> +        username.to_vec().into_boxed_slice(),
> +        ticket.to_vec().into_boxed_slice(),
> +    ))
> +}

(...)

> +fn listen_and_accept(hostname: &str, port: u16, timeout: u64) -> Result<(TcpStream, SocketAddr)> {
> +    let listener = TcpListener::bind((hostname, port))?;
> +    listener.set_nonblocking(true)?;

This turns the loop below into a busy-wait loop. Please use `poll` on
the listen socket for the timeout.

> +
> +    let now = Instant::now();
> +    loop {
> +        if now.elapsed().as_secs() > timeout {
> +            return Err(Error::new(
> +                ErrorKind::TimedOut,
> +                "timed out waiting for client",
> +            ));
> +        }
> +
> +        match listener.accept() {
> +            Err(err) => {
> +                if err.kind() == ErrorKind::WouldBlock {
> +                    continue;
> +                } else {
> +                    return Err(err);
> +                }
> +            }
> +            Ok(other) => return Ok(other),
> +        };
> +    }
> +}
> +
> +fn run_pty(cmd: &OsStr, params: clap::OsValues) -> Result<PTY> {
> +    let (mut pty, secondary_name) = PTY::new().map_err(io_err_other)?;
> +
> +    let mut filtered_env: HashMap<OsString, OsString> = std::env::vars_os()
> +        .filter(|&(ref k, _)| {
> +            k == "PATH" || k == "USER" || k == "HOME" || k == "LANG" || k == "LANGUAGE" ||
> +            k.to_string_lossy().starts_with("LC_")

Please use `OsStrExt`'s .as_bytes() instead of allocating a new `String`
here.

> +        }).collect();
> +    filtered_env.insert("TERM".into(), "xterm-256color".into());
> +
> +    let mut command = Command::new(cmd);
> +
> +    command
> +        .args(params)
> +        .env_clear()
> +        .envs(&filtered_env);
> +
> +    unsafe {
> +        command.pre_exec(move || {
> +            make_controlling_terminal(&secondary_name).map_err(io_err_other)?;
> +            Ok(())
> +        });
> +    }
> +
> +    command.spawn()?;
> +
> +    pty.set_size(80, 20).map_err(|x| x.as_errno().unwrap())?;

Another `unwrap` I don't find very convincing.
Consider using `into_io_error` from `proxmox::sys::error::SysError` or
`SysResult`.

> +    Ok(pty)
> +}
> +
> +const TCP: Token = Token(0);
> +const PTY: Token = Token(1);
> +
> +fn main() -> Result<()> {
> +    let matches = App::new("termproxy")
> +        .setting(AppSettings::TrailingVarArg)
> +        .arg(Arg::with_name("port").takes_value(true).required(true))
> +        .arg(
> +            Arg::with_name("authport")
> +                .takes_value(true)
> +                .long("authport"),
> +        )
> +        .arg(
> +            Arg::with_name("path")
> +                .takes_value(true)
> +                .long("path")
> +                .required(true),
> +        )
> +        .arg(Arg::with_name("perm").takes_value(true).long("perm"))
> +        .arg(Arg::with_name("cmd").multiple(true).required(true))
> +        .get_matches();
> +
> +    let port: u16 = matches .value_of("port") .unwrap() .parse() .map_err(io_err_other)?;
> +    let path = matches.value_of("path").unwrap();
> +    let perm: Option<&str> = matches.value_of("perm");
> +    let mut cmdparams = matches.values_of_os("cmd").unwrap();
> +    let cmd = cmdparams.next().unwrap();
> +    let authport: u16 = matches .value_of("authport") .unwrap_or("85") .parse() .map_err(io_err_other)?;
> +    let mut pty_buf = ByteBuffer::new();
> +    let mut tcp_buf = ByteBuffer::new();
> +
> +    let (mut stream, client) = listen_and_accept("localhost", port, 10)?;
> +
> +    let (username, ticket) = read_ticket_line(&mut stream, &mut pty_buf, 30)?;
> +    authenticate(&username, &ticket, path, perm, authport)?;
> +    stream.write_all("OK".as_bytes())?;

Use b"OK".

(...)

> +        while tcp_readable && !pty_buf.is_full() {
> +            let bytes = match pty_buf.read_from(&mut tcp_handle) {

You can handle `0` here:
                   Ok(0) => return Ok(()),

Skip the `let bytes =` binding and the `if bytes == 0` below.
Makes it a bit shorter.

> +                Ok(bytes) => bytes,
> +                Err(err) if err.kind() == ErrorKind::WouldBlock => {
> +                    tcp_readable = false;
> +                    break;
> +                }
> +                Err(err) => return Err(err),
> +            };
> +            if bytes == 0 {
> +                return Ok(());
> +            }
> +        }
> +
> +        while pty_readable && !tcp_buf.is_full() {
> +            let bytes = match tcp_buf.read_from(&mut pty) {
> +                Ok(bytes) => bytes,
> +                Err(err) if err.kind() == ErrorKind::WouldBlock => {
> +                    pty_readable = false;
> +                    break;
> +                }
> +                Err(err) => return Err(err),
> +            };
> +            if bytes == 0 {
> +                return Ok(());
> +            }
> +        }
> +
> +        while !tcp_buf.is_empty() && tcp_writable {
> +            let bytes = match tcp_handle.write(tcp_buf.get_data_slice()) {
> +                Ok(bytes) => bytes,

Similarly the let binding can go and the consume call put in the Ok case
here.

It's a bit more concise and this part here is quite long repetitive
code...

> +                Err(err) if err.kind() == ErrorKind::WouldBlock => {
> +                    tcp_writable = false;
> +                    break;
> +                }
> +                Err(err) => return Err(err),
> +            };
> +            tcp_buf.consume(bytes);
> +        }
> +
> +        while !pty_buf.is_empty() && pty_writable {
> +            if remaining == 0 {
> +                remaining = match process_queue(&mut pty_buf, &mut pty) {
> +                    Some(val) => val,
> +                    None => break,
> +                };
> +            }
> +            let len = min(remaining, pty_buf.data_size());
> +            let bytes = match pty.write(&pty_buf.get_data_slice()[0..len]) {
> +                Ok(bytes) => bytes,
> +                Err(err) if err.kind() == ErrorKind::WouldBlock => {
> +                    pty_writable = false;
> +                    break;
> +                }
> +                Err(err) => return Err(err),
> +            };
> +            remaining -= bytes;
> +            pty_buf.consume(bytes);
> +        }
> +    }
> +}