From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3A4D15B5D5 for ; Tue, 7 Jul 2020 12:11:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3286226936 for ; Tue, 7 Jul 2020 12:11:21 +0200 (CEST) Received-SPF: pass (proxmox.com: 212.186.127.180 is authorized to use 'w.bumiller@proxmox.com' in 'mfrom' identity (mechanism 'mx' matched)) receiver=firstgate.proxmox.com; identity=mailfrom; envelope-from="w.bumiller@proxmox.com"; helo=proxmox-new.maurer-it.com; client-ip=212.186.127.180 Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 472A62692A for ; Tue, 7 Jul 2020 12:11:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 49D07441EC for ; Tue, 7 Jul 2020 11:34:53 +0200 (CEST) Date: Tue, 7 Jul 2020 11:34:48 +0200 From: Wolfgang Bumiller To: Dominik Csapak Cc: pve-devel@pve.proxmox.com Message-ID: <20200707093448.m2vluxhktozzlptm@olga.proxmox.com> References: <20200706105721.9241-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200706105721.9241-1-d.csapak@proxmox.com> User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [main.rs] X-Mailman-Approved-At: Tue, 07 Jul 2020 12:27:43 +0200 Subject: Re: [pve-devel] [PATCH xtermjs] termproxy: rewrite in rust X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: PVE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Jul 2020 10:11:21 -0000 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 { > + 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 { > + 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` 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 { > + let (mut pty, secondary_name) = PTY::new().map_err(io_err_other)?; > + > + let mut filtered_env: HashMap = 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); > + } > + } > +}