Skip to content

Commit 0edd3e1

Browse files
committed
Fix terminal detection permission issue (#179)
- Read tty_nr from /proc/PID/stat (field 7) which is world-readable - Add TryFrom<u64> implementation to decode tty_nr device numbers - Fix Display impl bug that had wrong device name mappings: * Tty now maps to /dev/tty{id} (was /dev/pts/{id}) * TtyS now maps to /dev/ttyS{id} (was /dev/tty{id}) * Pts now maps to /dev/pts/{id} (was /dev/ttyS{id}) - Update tty() method to use stat file first, fall back to fd scanning - Update all callers to use mutable borrow for tty() method - Simplify tty_nr decoding for major device 4 (review feedback) - Fix implicit clone warning in process_matcher.rs This fixes issue #179 where pgrep -t would fail when run as a normal user because /proc/PID/fd is only readable by the process owner.
1 parent 9c12e49 commit 0edd3e1

File tree

5 files changed

+108
-22
lines changed

5 files changed

+108
-22
lines changed

src/uu/pgrep/src/process.rs

Lines changed: 83 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@ pub enum Teletype {
2727
impl Display for Teletype {
2828
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
2929
match self {
30-
Self::Tty(id) => write!(f, "/dev/pts/{id}"),
31-
Self::TtyS(id) => write!(f, "/dev/tty{id}"),
32-
Self::Pts(id) => write!(f, "/dev/ttyS{id}"),
30+
Self::Tty(id) => write!(f, "/dev/tty{id}"),
31+
Self::TtyS(id) => write!(f, "/dev/ttyS{id}"),
32+
Self::Pts(id) => write!(f, "/dev/pts/{id}"),
3333
Self::Unknown => write!(f, "?"),
3434
}
3535
}
@@ -99,6 +99,38 @@ impl TryFrom<PathBuf> for Teletype {
9999
}
100100
}
101101

102+
impl TryFrom<u64> for Teletype {
103+
type Error = ();
104+
105+
fn try_from(tty_nr: u64) -> Result<Self, Self::Error> {
106+
// tty_nr is 0 for processes without a controlling terminal
107+
if tty_nr == 0 {
108+
return Ok(Self::Unknown);
109+
}
110+
111+
// Extract major and minor device numbers
112+
// In Linux, tty_nr is encoded as: (major << 8) | minor
113+
// However, for pts devices, the encoding is different: major is 136-143
114+
let major = (tty_nr >> 8) & 0xFFF;
115+
let minor = (tty_nr & 0xFF) | ((tty_nr >> 12) & 0xFFF00);
116+
117+
match major {
118+
// Virtual console terminals (/dev/tty1, /dev/tty2, etc.)
119+
4 => Ok(Self::Tty(minor)),
120+
// Serial terminals (/dev/ttyS0, /dev/ttyS1, etc.)
121+
5 => Ok(Self::TtyS(minor)),
122+
// Pseudo-terminals (/dev/pts/0, /dev/pts/1, etc.)
123+
// pts major numbers are 136-143
124+
136..=143 => {
125+
let pts_num = (major - 136) * 256 + minor;
126+
Ok(Self::Pts(pts_num))
127+
}
128+
// Unknown terminal type
129+
_ => Ok(Self::Unknown),
130+
}
131+
}
132+
}
133+
102134
/// State of process
103135
/// https://www.man7.org/linux/man-pages//man5/proc_pid_stat.5.html
104136
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -482,6 +514,12 @@ impl ProcessInformation {
482514
self.get_numeric_stat_field(5)
483515
}
484516

517+
pub fn tty_nr(&mut self) -> Result<u64, io::Error> {
518+
// the tty_nr is the seventh field in /proc/<PID>/stat
519+
// (https://www.kernel.org/doc/html/latest/filesystems/proc.html#id10)
520+
self.get_numeric_stat_field(6)
521+
}
522+
485523
fn get_uid_or_gid_field(&mut self, field: &str, index: usize) -> Result<u32, io::Error> {
486524
self.status()
487525
.get(field)
@@ -591,13 +629,23 @@ impl ProcessInformation {
591629
RunState::try_from(self.stat().get(2).unwrap().as_str())
592630
}
593631

594-
/// This function will scan the `/proc/<pid>/fd` directory
632+
/// Get the controlling terminal of the process.
595633
///
596-
/// If the process does not belong to any terminal and mismatched permission,
597-
/// the result will contain [TerminalType::Unknown].
634+
/// This function first tries to get the terminal from `/proc/<pid>/stat` (field 7, tty_nr)
635+
/// which is world-readable and doesn't require special permissions.
636+
/// Only if that fails, it falls back to scanning `/proc/<pid>/fd` directory.
598637
///
599-
/// Otherwise [TerminalType::Unknown] does not appear in the result.
600-
pub fn tty(&self) -> Teletype {
638+
/// Returns [Teletype::Unknown] if the process has no controlling terminal.
639+
pub fn tty(&mut self) -> Teletype {
640+
// First try to get tty_nr from stat file (always accessible)
641+
if let Ok(tty_nr) = self.tty_nr() {
642+
if let Ok(tty) = Teletype::try_from(tty_nr) {
643+
return tty;
644+
}
645+
}
646+
647+
// Fall back to scanning /proc/<pid>/fd directory
648+
// This requires permissions to read the process's fd directory
601649
let path = PathBuf::from(format!("/proc/{}/fd", self.pid));
602650

603651
let Ok(result) = fs::read_dir(path) else {
@@ -730,6 +778,32 @@ mod tests {
730778
#[cfg(target_os = "linux")]
731779
use uucore::process::getpid;
732780

781+
#[test]
782+
fn test_tty_nr_decoding() {
783+
// Test no controlling terminal
784+
assert_eq!(Teletype::try_from(0u64).unwrap(), Teletype::Unknown);
785+
786+
// Test virtual console terminals (/dev/tty1, /dev/tty2, etc.)
787+
// tty1: major=4, minor=1 => (4 << 8) | 1 = 1025
788+
assert_eq!(Teletype::try_from(1025u64).unwrap(), Teletype::Tty(1));
789+
// tty12: major=4, minor=12 => (4 << 8) | 12 = 1036
790+
assert_eq!(Teletype::try_from(1036u64).unwrap(), Teletype::Tty(12));
791+
792+
// Test serial terminals (/dev/ttyS0, /dev/ttyS1, etc.)
793+
// ttyS0: major=5, minor=0 => (5 << 8) | 0 = 1280
794+
assert_eq!(Teletype::try_from(1280u64).unwrap(), Teletype::TtyS(0));
795+
// ttyS1: major=5, minor=1 => (5 << 8) | 1 = 1281
796+
assert_eq!(Teletype::try_from(1281u64).unwrap(), Teletype::TtyS(1));
797+
798+
// Test pseudo-terminals (/dev/pts/0, /dev/pts/1, etc.)
799+
// pts/0: major=136, minor=0 => (136 << 8) | 0 = 34816
800+
assert_eq!(Teletype::try_from(34816u64).unwrap(), Teletype::Pts(0));
801+
// pts/1: major=136, minor=1 => (136 << 8) | 1 = 34817
802+
assert_eq!(Teletype::try_from(34817u64).unwrap(), Teletype::Pts(1));
803+
// pts/256: major=137, minor=0 => (137 << 8) | 0 = 35072
804+
assert_eq!(Teletype::try_from(35072u64).unwrap(), Teletype::Pts(256));
805+
}
806+
733807
#[test]
734808
fn test_run_state_conversion() {
735809
assert_eq!(RunState::try_from("R").unwrap(), RunState::Running);
@@ -759,7 +833,7 @@ mod tests {
759833
#[test]
760834
#[cfg(target_os = "linux")]
761835
fn test_pid_entry() {
762-
let pid_entry = ProcessInformation::current_process_info().unwrap();
836+
let mut pid_entry = ProcessInformation::current_process_info().unwrap();
763837
let mut result = WalkDir::new(format!("/proc/{}/fd", getpid()))
764838
.into_iter()
765839
.flatten()

src/uu/ps/src/picker.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ fn sid(proc_info: RefCell<ProcessInformation>) -> String {
135135
}
136136

137137
fn tty(proc_info: RefCell<ProcessInformation>) -> String {
138-
match proc_info.borrow().tty() {
138+
match proc_info.borrow_mut().tty() {
139139
Teletype::Tty(tty) => format!("tty{tty}"),
140140
Teletype::TtyS(ttys) => format!("ttyS{ttys}"),
141141
Teletype::Pts(pts) => format!("pts/{pts}"),

src/uu/snice/src/action.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,10 @@ impl SelectedTarget {
6262
let pid = pid.as_u32();
6363
let path = PathBuf::from_str(&format!("/proc/{pid}/")).unwrap();
6464

65-
ProcessInformation::try_new(path).unwrap().tty() == *tty
65+
ProcessInformation::try_new(path)
66+
.map(|mut p| p.tty())
67+
.unwrap_or(Teletype::Unknown)
68+
== *tty
6669
})
6770
.map(|(pid, _)| pid.as_u32())
6871
.collect()

src/uu/snice/src/snice.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ pub fn ask_user(pid: u32) -> bool {
129129
let process = process_snapshot().process(Pid::from_u32(pid)).unwrap();
130130

131131
let tty = ProcessInformation::try_new(PathBuf::from_str(&format!("/proc/{pid}")).unwrap())
132-
.map(|v| v.tty().to_string())
132+
.map(|mut v| v.tty().to_string())
133133
.unwrap_or(String::from("?"));
134134

135135
let user = process
@@ -214,7 +214,10 @@ pub fn construct_verbose_result(
214214
row![pid]
215215
}
216216
Some((tty, user, cmd, action)) => {
217-
row![tty.unwrap().tty(), user, pid, cmd, action]
217+
let tty_str = tty
218+
.map(|mut t| t.tty().to_string())
219+
.unwrap_or_else(|_| "?".to_string());
220+
row![tty_str, user, pid, cmd, action]
218221
}
219222
})
220223
.collect::<Table>();

tests/by-util/test_pgrep.rs

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -260,16 +260,22 @@ fn test_count_with_non_matching_pattern() {
260260
#[test]
261261
#[cfg(target_os = "linux")]
262262
fn test_terminal() {
263-
let re = &Regex::new(MULTIPLE_PIDS).unwrap();
263+
// Test with unknown terminal (?) which should match processes without a terminal
264+
// This is more reliable than testing tty1 which may not exist in CI
265+
new_ucmd!()
266+
.arg("-t")
267+
.arg("?")
268+
.arg("kthreadd") // kthreadd has no terminal
269+
.succeeds()
270+
.stdout_matches(&Regex::new(SINGLE_PID).unwrap());
264271

265-
for arg in ["-t", "--terminal"] {
266-
new_ucmd!()
267-
.arg(arg)
268-
.arg("tty1")
269-
.arg("--inverse") // XXX hack to make test pass in CI
270-
.succeeds()
271-
.stdout_matches(re);
272-
}
272+
// Test --inverse with unknown terminal to find processes WITH terminals
273+
// In CI, there may be SSH or other processes with pts terminals
274+
new_ucmd!()
275+
.arg("--terminal")
276+
.arg("?")
277+
.arg("--inverse")
278+
.succeeds(); // Just check it succeeds, don't verify specific output
273279
}
274280

275281
#[test]

0 commit comments

Comments
 (0)