From 5e7d58650dd8f790a0857fa34e9d99bd07961678 Mon Sep 17 00:00:00 2001 From: Justin Tracey Date: Thu, 17 Feb 2022 00:58:34 -0500 Subject: [PATCH] fix null pointer derefs The code for creating a Passwd from the fields of the raw syscall result assumed that the syscall would return valid C strings in all non-error cases. This is not true, and at least one platform (Android) will populate the fields with null pointers where they are not supported. To fix this and prevent the error from happening again, this commit changes `cstr2string(ptr)` to check for a null pointer, and return an `Option`, with `None` being the null pointer case. While arguably it should be the caller's job to check for a null pointer before calling (since the safety precondition is that the pointer is to a valid C string), relying on the type checker to force remembering this edge case is safer in the long run. --- src/uu/id/src/id.rs | 18 ++++++++++----- src/uu/pinky/src/pinky.rs | 32 ++++++++++++++++++-------- src/uucore/src/lib/features/entries.rs | 23 ++++++++++-------- tests/by-util/test_pinky.rs | 7 ++++-- 4 files changed, 53 insertions(+), 27 deletions(-) diff --git a/src/uu/id/src/id.rs b/src/uu/id/src/id.rs index 3bb9ce8c9..714ade035 100644 --- a/src/uu/id/src/id.rs +++ b/src/uu/id/src/id.rs @@ -508,15 +508,15 @@ fn pline(possible_uid: Option) { println!( "{}:{}:{}:{}:{}:{}:{}:{}:{}:{}", pw.name, - pw.user_passwd, + pw.user_passwd.unwrap_or_default(), pw.uid, pw.gid, - pw.user_access_class, + pw.user_access_class.unwrap_or_default(), pw.passwd_change_time, pw.expiration, - pw.user_info, - pw.user_dir, - pw.user_shell + pw.user_info.unwrap_or_default(), + pw.user_dir.unwrap_or_default(), + pw.user_shell.unwrap_or_default() ); } @@ -527,7 +527,13 @@ fn pline(possible_uid: Option) { println!( "{}:{}:{}:{}:{}:{}:{}", - pw.name, pw.user_passwd, pw.uid, pw.gid, pw.user_info, pw.user_dir, pw.user_shell + pw.name, + pw.user_passwd.unwrap_or_default(), + pw.uid, + pw.gid, + pw.user_info.unwrap_or_default(), + pw.user_dir.unwrap_or_default(), + pw.user_shell.unwrap_or_default() ); } diff --git a/src/uu/pinky/src/pinky.rs b/src/uu/pinky/src/pinky.rs index 437c20cf5..5029caa24 100644 --- a/src/uu/pinky/src/pinky.rs +++ b/src/uu/pinky/src/pinky.rs @@ -245,12 +245,16 @@ 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(); +fn gecos_to_fullname(pw: &Passwd) -> Option { + let mut gecos = if let Some(gecos) = &pw.user_info { + gecos.clone() + } else { + return None; + }; if let Some(n) = gecos.find(',') { gecos.truncate(n); } - gecos.replace('&', &pw.name.capitalize()) + Some(gecos.replace('&', &pw.name.capitalize())) } impl Pinky { @@ -278,8 +282,13 @@ impl Pinky { print!("{1:<8.0$}", utmpx::UT_NAMESIZE, ut.user()); if self.include_fullname { - if let Ok(pw) = Passwd::locate(ut.user().as_ref()) { - print!(" {:<19.19}", gecos_to_fullname(&pw)); + let fullname = if let Ok(pw) = Passwd::locate(ut.user().as_ref()) { + gecos_to_fullname(&pw) + } else { + None + }; + if let Some(fullname) = fullname { + print!(" {:<19.19}", fullname); } else { print!(" {:19}", " ???"); } @@ -341,13 +350,16 @@ 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!(" {}", gecos_to_fullname(&pw)); + let fullname = gecos_to_fullname(&pw).unwrap_or_default(); + let user_dir = pw.user_dir.unwrap_or_default(); + let user_shell = pw.user_shell.unwrap_or_default(); + println!(" {}", fullname); if self.include_home_and_shell { - print!("Directory: {:<29}", pw.user_dir); - println!("Shell: {}", pw.user_shell); + print!("Directory: {:<29}", user_dir); + println!("Shell: {}", user_shell); } if self.include_project { - let mut p = PathBuf::from(&pw.user_dir); + let mut p = PathBuf::from(&user_dir); p.push(".project"); if let Ok(f) = File::open(p) { print!("Project: "); @@ -355,7 +367,7 @@ impl Pinky { } } if self.include_plan { - let mut p = PathBuf::from(&pw.user_dir); + let mut p = PathBuf::from(&user_dir); p.push(".plan"); if let Ok(f) = File::open(p) { println!("Plan:"); diff --git a/src/uucore/src/lib/features/entries.rs b/src/uucore/src/lib/features/entries.rs index 0366355d4..03c5a56cf 100644 --- a/src/uucore/src/lib/features/entries.rs +++ b/src/uucore/src/lib/features/entries.rs @@ -147,16 +147,16 @@ pub struct Passwd { /// AKA passwd.pw_gid pub gid: gid_t, /// AKA passwd.pw_gecos - pub user_info: String, + pub user_info: Option, /// AKA passwd.pw_shell - pub user_shell: String, + pub user_shell: Option, /// AKA passwd.pw_dir - pub user_dir: String, + pub user_dir: Option, /// AKA passwd.pw_passwd - pub user_passwd: String, + pub user_passwd: Option, /// AKA passwd.pw_class #[cfg(any(target_os = "freebsd", target_vendor = "apple"))] - pub user_access_class: String, + pub user_access_class: Option, /// AKA passwd.pw_change #[cfg(any(target_os = "freebsd", target_vendor = "apple"))] pub passwd_change_time: time_t, @@ -166,8 +166,13 @@ pub struct Passwd { } /// SAFETY: ptr must point to a valid C string. -unsafe fn cstr2string(ptr: *const c_char) -> String { - CStr::from_ptr(ptr).to_string_lossy().into_owned() +/// Returns None if ptr is null. +unsafe fn cstr2string(ptr: *const c_char) -> Option { + if !ptr.is_null() { + Some(CStr::from_ptr(ptr).to_string_lossy().into_owned()) + } else { + None + } } impl Passwd { @@ -175,7 +180,7 @@ impl Passwd { /// the function runs. That means PW_LOCK must be held. unsafe fn from_raw(raw: passwd) -> Self { Self { - name: cstr2string(raw.pw_name), + name: cstr2string(raw.pw_name).expect("passwd without name"), uid: raw.pw_uid, gid: raw.pw_gid, user_info: cstr2string(raw.pw_gecos), @@ -243,7 +248,7 @@ impl Group { /// the function runs. That means PW_LOCK must be held. unsafe fn from_raw(raw: group) -> Self { Self { - name: cstr2string(raw.gr_name), + name: cstr2string(raw.gr_name).expect("group without name"), gid: raw.gr_gid, } } diff --git a/tests/by-util/test_pinky.rs b/tests/by-util/test_pinky.rs index c036d449f..1da93ee42 100644 --- a/tests/by-util/test_pinky.rs +++ b/tests/by-util/test_pinky.rs @@ -24,11 +24,14 @@ fn test_capitalize() { fn test_long_format() { let login = "root"; let pw: Passwd = Passwd::locate(login).unwrap(); - let real_name = pw.user_info.replace('&', &pw.name.capitalize()); + let user_info = pw.user_info.unwrap_or_default(); + let user_dir = pw.user_dir.unwrap_or_default(); + let user_shell = pw.user_shell.unwrap_or_default(); + let real_name = user_info.replace('&', &pw.name.capitalize()); let ts = TestScenario::new(util_name!()); ts.ucmd().arg("-l").arg(login).succeeds().stdout_is(format!( "Login name: {:<28}In real life: {}\nDirectory: {:<29}Shell: {}\n\n", - login, real_name, pw.user_dir, pw.user_shell + login, real_name, user_dir, user_shell )); ts.ucmd()