Merge pull request #3155 from jtracey/gecos-off-by-one

pinky: fix off-by-one in GECOS parsing

Author: sylvestre

.github/workflows/CICD.yml

@@ -651,17 +651,6 @@ jobs:
           *-pc-windows-msvc) STRIP="" ;;
         esac;
         outputs STRIP
-    - name: Install/setup prerequisites
-      shell: bash
-      run: |
-        ## Install/setup prerequisites
-        case '${{ matrix.job.target }}' in
-          arm-unknown-linux-gnueabihf) sudo apt-get -y update ; sudo apt-get -y install gcc-arm-linux-gnueabihf ;;
-          aarch64-unknown-linux-gnu) sudo apt-get -y update ; sudo apt-get -y install gcc-aarch64-linux-gnu ;;
-        esac
-        case '${{ matrix.job.os }}' in
-          macos-latest) brew install coreutils ;; # needed for testing
-        esac
     - name: Create all needed build/work directories
       shell: bash
       run: |
@@ -680,6 +669,21 @@ jobs:
         case '${{ matrix.job.os }}' in
           macos-latest) brew install coreutils ;; # needed for testing
         esac
+        case '${{ matrix.job.os }}' in
+          ubuntu-*)
+            # pinky is a tool to show logged-in users from utmp, and gecos fields from /etc/passwd.
+            # In GitHub Action *nix VMs, no accounts log in, even the "runner" account that runs the commands. The account also has empty gecos fields.
+            # To work around this for pinky tests, we create a fake login entry for the GH runner account...
+            FAKE_UTMP='[7] [999999] [tty2] [runner] [tty2] [] [0.0.0.0] [2022-02-22T22:22:22,222222+00:00]'
+            # ... by dumping the login records, adding our fake line, then reverse dumping ...
+            (utmpdump /var/run/utmp ; echo $FAKE_UTMP) | sudo utmpdump -r -o /var/run/utmp
+            # ... and add a full name to each account with a gecos field but no full name.
+            sudo sed -i 's/:,/:runner name,/' /etc/passwd
+            # We also create a couple optional files pinky looks for
+            touch /home/runner/.project
+            echo "foo" > /home/runner/.plan
+            ;;
+        esac
     - name: rust toolchain ~ install
       uses: actions-rs/toolchain@v1
       with:
@@ -949,6 +953,21 @@ jobs:
         case '${{ matrix.job.os }}' in
           macos-latest) brew install coreutils ;; # needed for testing
         esac
+        case '${{ matrix.job.os }}' in
+          ubuntu-latest)
+            # pinky is a tool to show logged-in users from utmp, and gecos fields from /etc/passwd.
+            # In GitHub Action *nix VMs, no accounts log in, even the "runner" account that runs the commands. The account also has empty gecos fields.
+            # To work around this for pinky tests, we create a fake login entry for the GH runner account...
+            FAKE_UTMP='[7] [999999] [tty2] [runner] [tty2] [] [0.0.0.0] [2022-02-22T22:22:22,222222+00:00]'
+            # ... by dumping the login records, adding our fake line, then reverse dumping ...
+            (utmpdump /var/run/utmp ; echo $FAKE_UTMP) | sudo utmpdump -r -o /var/run/utmp
+            # ... and add a full name to each account with a gecos field but no full name.
+            sudo sed -i 's/:,/:runner name,/' /etc/passwd
+            # We also create a couple optional files pinky looks for
+            touch /home/runner/.project
+            echo "foo" > /home/runner/.plan
+            ;;
+        esac
     - name: rust toolchain ~ install
       uses: actions-rs/toolchain@v1
       with:

src/uu/pinky/src/pinky.rs

@@ -22,8 +22,6 @@ use clap::{crate_version, App, AppSettings, Arg};
 use std::path::PathBuf;
 use uucore::{format_usage, InvalidEncodingHandling};
 
-const BUFSIZE: usize = 1024;
-
 static ABOUT: &str = "pinky - lightweight finger";
 const USAGE: &str = "{} [OPTION]... [USER]...";
 
@@ -239,6 +237,14 @@ fn time_string(ut: &Utmpx) -> String {
     time::strftime("%b %e %H:%M", &ut.login_time()).unwrap() // LC_ALL=C
 }
 
+fn gecos_to_fullname(pw: &Passwd) -> String {
+    let mut gecos = pw.user_info.clone();
+    if let Some(n) = gecos.find(',') {
+        gecos.truncate(n);
+    }
+    gecos.replace('&', &pw.name.capitalize())
+}
+
 impl Pinky {
     fn print_entry(&self, ut: &Utmpx) -> std::io::Result<()> {
         let mut pts_path = PathBuf::from("/dev");
@@ -265,11 +271,7 @@ impl Pinky {
 
         if self.include_fullname {
             if let Ok(pw) = Passwd::locate(ut.user().as_ref()) {
-                let mut gecos = pw.user_info;
-                if let Some(n) = gecos.find(',') {
-                    gecos.truncate(n + 1);
-                }
-                print!(" {:<19.19}", gecos.replace('&', &pw.name.capitalize()));
+                print!(" {:<19.19}", gecos_to_fullname(&pw));
             } else {
                 print!(" {:19}", "        ???");
             }
@@ -331,7 +333,7 @@ impl Pinky {
         for u in &self.names {
             print!("Login name: {:<28}In real life: ", u);
             if let Ok(pw) = Passwd::locate(u.as_str()) {
-                println!(" {}", pw.user_info.replace('&', &pw.name.capitalize()));
+                println!(" {}", gecos_to_fullname(&pw));
                 if self.include_home_and_shell {
                     print!("Directory: {:<29}", pw.user_dir);
                     println!("Shell:  {}", pw.user_shell);
@@ -362,12 +364,8 @@ impl Pinky {
 
 fn read_to_console<F: Read>(f: F) {
     let mut reader = BufReader::new(f);
-    let mut iobuf = [0_u8; BUFSIZE];
-    while let Ok(n) = reader.read(&mut iobuf) {
-        if n == 0 {
-            break;
-        }
-        let s = String::from_utf8_lossy(&iobuf);
-        print!("{}", s);
+    let mut iobuf = Vec::new();
+    if reader.read_to_end(&mut iobuf).is_ok() {
+        print!("{}", String::from_utf8_lossy(&iobuf));
     }
 }

tests/by-util/test_pinky.rs

@@ -44,7 +44,14 @@ fn test_long_format() {
 #[cfg(unix)]
 #[test]
 fn test_long_format_multiple_users() {
-    let args = ["-l", "root", "root", "root"];
+    // multiple instances of one account we know exists,
+    // the account of the test runner,
+    // and an account that (probably) doesn't exist
+    let runner = match std::env::var("USER") {
+        Ok(user) => user,
+        Err(_) => "".to_string(),
+    };
+    let args = ["-l", "root", "root", "root", &runner, "no_such_user"];
     let ts = TestScenario::new(util_name!());
     let expect = unwrap_or_return!(expected_result(&ts, &args));
 

tests/by-util/test_users.rs

@@ -7,6 +7,7 @@ fn test_users_no_arg() {
 
 #[test]
 #[cfg(any(target_vendor = "apple", target_os = "linux"))]
+#[ignore = "issue #3219"]
 fn test_users_check_name() {
     #[cfg(target_os = "linux")]
     let util_name = util_name!();

tests/by-util/test_who.rs

@@ -9,6 +9,7 @@ use crate::common::util::*;
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_count() {
     let ts = TestScenario::new(util_name!());
     for opt in &["-q", "--count", "--c"] {
@@ -29,6 +30,7 @@ fn test_boot() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_heading() {
     let ts = TestScenario::new(util_name!());
     for opt in &["-H", "--heading", "--head"] {
@@ -47,6 +49,7 @@ fn test_heading() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_short() {
     let ts = TestScenario::new(util_name!());
     for opt in &["-s", "--short", "--s"] {
@@ -108,6 +111,7 @@ fn test_time() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_mesg() {
     // -T, -w, --mesg
     //     add user's message status as +, - or ?
@@ -150,6 +154,7 @@ fn test_too_many_args() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_users() {
     let ts = TestScenario::new(util_name!());
     for opt in &["-u", "--users", "--us"] {
@@ -175,6 +180,7 @@ fn test_users() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_lookup() {
     let opt = "--lookup";
     let ts = TestScenario::new(util_name!());
@@ -194,6 +200,7 @@ fn test_dead() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_all_separately() {
     if cfg!(target_os = "macos") {
         // TODO: fix `-u`, see: test_users
@@ -211,6 +218,7 @@ fn test_all_separately() {
 
 #[cfg(unix)]
 #[test]
+#[ignore = "issue #3219"]
 fn test_all() {
     if cfg!(target_os = "macos") {
         // TODO: fix `-u`, see: test_users