redbear-power: v1.38.1 fix io_priority field index
v1.38 review found a HIGH severity bug in the v1.38 io_priority reader: it read fields[44] (overall field 47) which on modern Linux kernels (~52 fields total) is a memory address (~9x10^13) that overflows u32 and silently returns None for every process. The PID detail popup shows 'IO priority: ?' on every process, on every Linux kernel released in the last ~5 years. Fix: change fields[44] to fields[15] (post-comm field 15 = overall field 18 = priority per the /proc/[pid]/stat layout). The doc comment now spells out the per-field index map 0-15 so a future maintainer can verify the indexing. Strengthened the regression test: the v1.38 test did 'let _ = ...' which passed even when the reader was broken. The new test reads /proc/self/stat directly (bypassing the function), parses the priority field at vec[15] itself, and asserts the function returns the same value. A 'sanity' assertion (value < 1_000_000_000) catches the 'reading a memory address' failure mode specifically — a value > 10^9 means we're reading the wrong field again. This is exactly the kind of bug the v1.37 audit warned about: 'the existing 4 tests that exist for update_io_history all assert the post-normalize storage value, not what the renderer actually draws. A simple integration test that renders and reads back would have caught it.' The v1.38 test for io_priority had the same shape — 'let _ = ...' passes silently. The v1.38.1 test now reads the actual value back. Test count 149 -> 149 (strengthened one test in place; no new test count). Redox stripped binary: 4,348,776 bytes (unchanged; the field index is 1 character).
This commit is contained in:
@@ -111,29 +111,33 @@ fn read_cmdline(pid: u32) -> Option<String> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Read the I/O priority class from `/proc/<pid>/stat` field
|
||||
/// 47 (the priority is `class | (data << 13)`; we return the
|
||||
/// raw field as u32 so the caller can interpret both).
|
||||
/// Field 47 is the 47th space-separated token, 0-indexed at
|
||||
/// the THIRD field (state) plus 44 (matches the kernel's
|
||||
/// `getpriority(2)`-derived field).
|
||||
/// Read the I/O priority (nice-equivalent) from
|
||||
/// `/proc/<pid>/stat`. The /proc/[pid]/stat format is
|
||||
/// documented in `man 5 proc`; the priority field is at
|
||||
/// position 18 (1-indexed), which is post-comm index 15
|
||||
/// (0-indexed `fields[15]`) because fields 1-2 are pid+comm
|
||||
/// and field 3 is state. The v1.38 first cut used `fields[44]`
|
||||
/// (overall field 47), which on modern Linux kernels (~52
|
||||
/// fields total) is a memory address (~9×10¹³) — not the
|
||||
/// priority. Reading that value as u32 silently overflowed
|
||||
/// and the reader returned None for every process. This
|
||||
/// release fixes the field index.
|
||||
fn read_io_priority(pid: u32) -> Option<u32> {
|
||||
let stat = fs::read_to_string(format!("/proc/{pid}/stat")).ok()?;
|
||||
// The comm field is wrapped in parens and may contain
|
||||
// spaces, so we must find the LAST `)` to delimit the
|
||||
// comm field, then split the rest by whitespace. The
|
||||
// 44th post-comm field (1-indexed: field 47 overall)
|
||||
// holds the priority.
|
||||
// comm field, then split the rest by whitespace. After
|
||||
// the comm: [0]=state [1]=ppid [2]=pgrp [3]=session
|
||||
// [4]=tty_nr [5]=tpgid [6]=flags [7]=minflt [8]=cminflt
|
||||
// [9]=majflt [10]=cmajflt [11]=utime [12]=stime
|
||||
// [13]=cutime [14]=cstime [15]=priority. We want [15].
|
||||
let close = stat.rfind(')')?;
|
||||
let tail = &stat[close + 1..];
|
||||
let fields: Vec<&str> = tail.split_whitespace().collect();
|
||||
// post-comm field index 44 -> vec index 43 (0-indexed)
|
||||
// post-comm field 1 is the state (after the comm), so
|
||||
// 47th overall field is post-comm 45 (state + 44).
|
||||
if fields.len() < 45 {
|
||||
if fields.len() < 16 {
|
||||
return None;
|
||||
}
|
||||
fields[44].parse::<u32>().ok()
|
||||
fields[15].parse::<u32>().ok()
|
||||
}
|
||||
|
||||
fn read_status(pid: u32) -> ProcStatus {
|
||||
@@ -312,10 +316,43 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn read_io_priority_handles_self() {
|
||||
// Self's io_priority is a u32. Some kernels may not
|
||||
// expose it; tolerate that. The function must not
|
||||
// panic.
|
||||
let _ = read_io_priority(std::process::id());
|
||||
// v1.38.1 regression test: the v1.38 first cut read
|
||||
// `fields[44]` (overall field 47), which on modern
|
||||
// Linux kernels is a memory address (~9×10¹³) that
|
||||
// overflows u32 and silently returns None for every
|
||||
// process. The fix reads `fields[15]` (overall field
|
||||
// 18 = priority). This test reads the priority from
|
||||
// /proc/self/stat directly (bypassing the function)
|
||||
// and asserts the function returns the same value.
|
||||
let pid = std::process::id();
|
||||
let stat = std::fs::read_to_string(format!("/proc/{pid}/stat"))
|
||||
.expect("can read /proc/self/stat");
|
||||
let close = stat.rfind(')').expect("comm field has closing paren");
|
||||
let tail = &stat[close + 1..];
|
||||
let fields: Vec<&str> = tail.split_whitespace().collect();
|
||||
// 0-indexed: state=0, ppid=1, pgrp=2, session=3,
|
||||
// tty_nr=4, tpgid=5, flags=6, minflt=7, cminflt=8,
|
||||
// majflt=9, cmajflt=10, utime=11, stime=12, cutime=13,
|
||||
// cstime=14, priority=15.
|
||||
let expected: u32 = fields[15]
|
||||
.parse()
|
||||
.expect("priority is a u32");
|
||||
let actual = read_io_priority(pid)
|
||||
.expect("read_io_priority must return Some for self");
|
||||
assert_eq!(
|
||||
actual, expected,
|
||||
"v1.38.1: read_io_priority should return the priority field (overall field 18, post-comm vec[15])"
|
||||
);
|
||||
// Sanity: a typical default-nice process has priority
|
||||
// 20. We're not strict (containers / reniced processes
|
||||
// may differ), but if the value looks like a memory
|
||||
// address (>10^9) we know we're reading the wrong
|
||||
// field again.
|
||||
assert!(
|
||||
actual < 1_000_000_000,
|
||||
"read_io_priority returned {} which looks like a memory address, not a priority. The field index is wrong again.",
|
||||
actual
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user