From a4fc2b5106ad1a9226c6c396b5bb2ea191ac9814 Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Thu, 13 May 2021 10:17:57 +0200 Subject: [PATCH] who: fix `--lookup` This closes #2181. `who --lookup` is failing with a runtime panic (double free). Since `crate::dns-lookup` already includes a safe wrapper for `getaddrinfo` I used this crate instead of further debugging the existing code in utmpx::canon_host(). * It was neccessary to remove the version constraint for libc in uucore. --- Cargo.lock | 29 +++++++++++-- src/uu/pinky/src/pinky.rs | 15 ++----- src/uu/who/src/who.rs | 16 ++----- src/uucore/Cargo.toml | 1 + src/uucore/src/lib/features/utmpx.rs | 65 +++++++++++++--------------- tests/by-util/test_pinky.rs | 17 ++++++++ tests/by-util/test_who.rs | 1 - 7 files changed, 79 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d942c04d4..77957de80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -576,6 +576,18 @@ dependencies = [ "generic-array", ] +[[package]] +name = "dns-lookup" +version = "1.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "093d88961fd18c4ecacb8c80cd0b356463ba941ba11e0e01f9cf5271380b79dc" +dependencies = [ + "cfg-if 1.0.0", + "libc", + "socket2", + "winapi 0.3.9", +] + [[package]] name = "dunce" version = "1.0.1" @@ -1445,6 +1457,17 @@ dependencies = [ "maybe-uninit", ] +[[package]] +name = "socket2" +version = "0.3.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "122e570113d28d773067fab24266b66753f6ea915758651696b6e35e49f88d6e" +dependencies = [ + "cfg-if 1.0.0", + "libc", + "winapi 0.3.9", +] + [[package]] name = "strsim" version = "0.8.0" @@ -1820,11 +1843,9 @@ name = "uu_df" version = "0.0.6" dependencies = [ "clap", - "libc", "number_prefix", "uucore", "uucore_procs", - "winapi 0.3.9", ] [[package]] @@ -2407,8 +2428,6 @@ name = "uu_stat" version = "0.0.6" dependencies = [ "clap", - "libc", - "time", "uucore", "uucore_procs", ] @@ -2672,6 +2691,7 @@ name = "uucore" version = "0.0.8" dependencies = [ "data-encoding", + "dns-lookup", "dunce", "getopts", "lazy_static", @@ -2682,6 +2702,7 @@ dependencies = [ "thiserror", "time", "wild", + "winapi 0.3.9", ] [[package]] diff --git a/src/uu/pinky/src/pinky.rs b/src/uu/pinky/src/pinky.rs index e116a2382..f0ab44e5f 100644 --- a/src/uu/pinky/src/pinky.rs +++ b/src/uu/pinky/src/pinky.rs @@ -286,17 +286,10 @@ impl Pinky { print!(" {}", time_string(&ut)); - if self.include_where && !ut.host().is_empty() { - let ut_host = ut.host(); - let mut res = ut_host.splitn(2, ':'); - let host = match res.next() { - Some(_) => ut.canon_host().unwrap_or_else(|_| ut_host.clone()), - None => ut_host.clone(), - }; - match res.next() { - Some(d) => print!(" {}:{}", host, d), - None => print!(" {}", host), - } + let mut s = ut.host(); + if self.include_where && !s.is_empty() { + s = safe_unwrap!(ut.canon_host()); + print!(" {}", s); } println!(); diff --git a/src/uu/who/src/who.rs b/src/uu/who/src/who.rs index ba1360eff..aef23b3a2 100644 --- a/src/uu/who/src/who.rs +++ b/src/uu/who/src/who.rs @@ -548,20 +548,10 @@ impl Who { " ?".into() }; - let mut buf = vec![]; - let ut_host = ut.host(); - let mut res = ut_host.splitn(2, ':'); - if let Some(h) = res.next() { - if self.do_lookup { - buf.push(ut.canon_host().unwrap_or_else(|_| h.to_owned())); - } else { - buf.push(h.to_owned()); - } + let mut s = ut.host(); + if self.do_lookup { + s = safe_unwrap!(ut.canon_host()); } - if let Some(h) = res.next() { - buf.push(h.to_owned()); - } - let s = buf.join(":"); let hoststr = if s.is_empty() { s } else { format!("({})", s) }; self.print_line( diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index da51f7ca4..85efe0434 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -16,6 +16,7 @@ edition = "2018" path="src/lib/lib.rs" [dependencies] +dns-lookup = "1.0.5" dunce = "1.0.0" getopts = "<= 0.2.21" wild = "2.0.4" diff --git a/src/uucore/src/lib/features/utmpx.rs b/src/uucore/src/lib/features/utmpx.rs index 0308d8a5e..96db33c35 100644 --- a/src/uucore/src/lib/features/utmpx.rs +++ b/src/uucore/src/lib/features/utmpx.rs @@ -188,47 +188,40 @@ impl Utmpx { /// Canonicalize host name using DNS pub fn canon_host(&self) -> IOResult { - const AI_CANONNAME: libc::c_int = 0x2; let host = self.host(); - let host = host.split(':').next().unwrap(); - let hints = libc::addrinfo { - ai_flags: AI_CANONNAME, - ai_family: 0, - ai_socktype: 0, - ai_protocol: 0, - ai_addrlen: 0, - ai_addr: ptr::null_mut(), - ai_canonname: ptr::null_mut(), - ai_next: ptr::null_mut(), - }; - let c_host = CString::new(host).unwrap(); - let mut res = ptr::null_mut(); - let status = unsafe { - libc::getaddrinfo( - c_host.as_ptr(), - ptr::null(), - &hints as *const _, - &mut res as *mut _, - ) - }; - if status == 0 { - let info: libc::addrinfo = unsafe { ptr::read(res as *const _) }; - // http://lists.gnu.org/archive/html/bug-coreutils/2006-09/msg00300.html - // says Darwin 7.9.0 getaddrinfo returns 0 but sets - // res->ai_canonname to NULL. - let ret = if info.ai_canonname.is_null() { - Ok(String::from(host)) - } else { - Ok(unsafe { CString::from_raw(info.ai_canonname).into_string().unwrap() }) + + // TODO: change to use `split_once` when MSRV hits 1.52.0 + // let (hostname, display) = host.split_once(':').unwrap_or((&host, "")); + let mut h = host.split(':'); + let hostname = h.next().unwrap_or(&host); + let display = h.next().unwrap_or(""); + + if !hostname.is_empty() { + extern crate dns_lookup; + use dns_lookup::{getaddrinfo, AddrInfoHints}; + + const AI_CANONNAME: i32 = 0x2; + let hints = AddrInfoHints { + flags: AI_CANONNAME, + ..AddrInfoHints::default() }; - unsafe { - libc::freeaddrinfo(res); + let sockets = getaddrinfo(Some(&hostname), None, Some(hints)) + .unwrap() + .collect::>>()?; + for socket in sockets { + if let Some(ai_canonname) = socket.canonname { + return Ok(if display.is_empty() { + ai_canonname + } else { + format!("{}:{}", ai_canonname, display) + }); + } } - ret - } else { - Err(IOError::last_os_error()) } + + Ok(host.to_string()) } + pub fn iter_all_records() -> UtmpxIter { UtmpxIter } diff --git a/tests/by-util/test_pinky.rs b/tests/by-util/test_pinky.rs index 1a7ef8b61..904a05f93 100644 --- a/tests/by-util/test_pinky.rs +++ b/tests/by-util/test_pinky.rs @@ -98,6 +98,23 @@ fn test_short_format_q() { assert_eq!(v_actual, v_expect); } +#[cfg(target_os = "linux")] +#[test] +fn test_no_flag() { + let scene = TestScenario::new(util_name!()); + + let actual = scene.ucmd().succeeds().stdout_move_str(); + let expect = scene + .cmd_keepenv(util_name!()) + .env("LANGUAGE", "C") + .succeeds() + .stdout_move_str(); + + let v_actual: Vec<&str> = actual.split_whitespace().collect(); + let v_expect: Vec<&str> = expect.split_whitespace().collect(); + assert_eq!(v_actual, v_expect); +} + #[cfg(target_os = "linux")] fn expected_result(args: &[&str]) -> String { TestScenario::new(util_name!()) diff --git a/tests/by-util/test_who.rs b/tests/by-util/test_who.rs index 8aeecfb55..a5637f23a 100644 --- a/tests/by-util/test_who.rs +++ b/tests/by-util/test_who.rs @@ -162,7 +162,6 @@ fn test_users() { #[cfg(target_os = "linux")] #[test] -#[ignore] fn test_lookup() { for opt in vec!["--lookup"] { new_ucmd!()