modules/disk-load/cmd/disk-load/cpu_process.go

@@ -25,22 +25,22 @@ type ProcessCPU struct {
 
 // procTicks is a process's utime+stime ticks and thread count from its /proc/<pid>/stat, whose comm
 // field may hold spaces and parentheses, so the fields are counted from the last ")".
-func parseProcStat(s string) (comm string, ticks uint64, threads int, ok bool) {
+func parseProcStat(s string) (comm string, ticks uint64, threads int, start string, ok bool) {
 	l, r := strings.Index(s, "("), strings.LastIndex(s, ")")
 	if l < 0 || r < l {
-		return "", 0, 0, false
+		return "", 0, 0, "", false
 	}
 	f := strings.Fields(s[r+1:])
-	if len(f) < 18 {
-		return "", 0, 0, false
+	if len(f) < 20 {
+		return "", 0, 0, "", false
 	}
 	ut, e1 := strconv.ParseUint(f[11], 10, 64)
 	st, e2 := strconv.ParseUint(f[12], 10, 64)
 	th, e3 := strconv.Atoi(f[17])
 	if e1 != nil || e2 != nil || e3 != nil {
-		return "", 0, 0, false
+		return "", 0, 0, "", false
 	}
-	return s[l+1 : r], ut + st, th, true
+	return s[l+1 : r], ut + st, th, f[19], true
 }
 
 // cgroupOfUnit is the cgroup path (relative to /sys/fs/cgroup) a unit name or a path names.
@@ -48,6 +48,9 @@ func (m *Machine) cgroupOfUnit(unit string) (string, error) {
 	base := m.path("/sys/fs/cgroup")
 	if strings.Contains(unit, "/") {
 		rel := strings.Trim(strings.TrimPrefix(unit, "/sys/fs/cgroup"), "/")
+		if strings.Contains(rel, "..") || filepath.Clean("/"+rel) != "/"+rel {
+			return "", fmt.Errorf("cgroup %q is not a path under /sys/fs/cgroup", unit)
+		}
 		if rel == "" || !m.exists("/sys/fs/cgroup/"+rel) {
 			return "", fmt.Errorf("no cgroup %q", unit)
 		}
@@ -75,6 +78,7 @@ type procReading struct {
 	comm    string
 	threads int
 	pid     int
+	start   string // starttime, to tell a reused pid
 }
 
 // unitProcs reads every process in a cgroup and its child cgroups: its ticks, name and threads.
@@ -90,9 +94,9 @@ func (m *Machine) unitProcs(cgroup string) map[int]procReading {
 			if err != nil {
 				continue
 			}
-			comm, ticks, th, ok := parseProcStat(m.read("/proc/" + f + "/stat"))
+			comm, ticks, th, start, ok := parseProcStat(m.read("/proc/" + f + "/stat"))
 			if ok {
-				out[pid] = procReading{ticks: ticks, comm: comm, threads: th, pid: pid}
+				out[pid] = procReading{ticks: ticks, comm: comm, threads: th, pid: pid, start: start}
 			}
 		}
 		return nil
@@ -100,9 +104,11 @@ func (m *Machine) unitProcs(cgroup string) map[int]procReading {
 	return out
 }
 
-// commandLine is a process's command line, its NULs as spaces, cut to 120 characters.
+// commandLine is a process's program path (argv[0], cut to 120 characters), never its arguments:
+// they can carry a password.
 func (m *Machine) commandLine(pid int) string {
-	s := strings.TrimSpace(strings.ReplaceAll(m.read("/proc/"+strconv.Itoa(pid)+"/cmdline"), "\x00", " "))
+	s, _, _ := strings.Cut(m.read("/proc/"+strconv.Itoa(pid)+"/cmdline"), "\x00")
+	s = strings.TrimSpace(s)
 	if r := []rune(s); len(r) > 120 {
 		s = string(r[:120])
 	}
@@ -116,7 +122,7 @@ func (m *Machine) topProcessCPU(a, b map[int]procReading, window time.Duration,
 	var sum float64
 	for pid, end := range b {
 		start, ok := a[pid]
-		if !ok || end.ticks <= start.ticks {
+		if !ok || end.start != start.start || end.ticks <= start.ticks {
 			continue
 		}
 		pct := float64(end.ticks-start.ticks) / clockTicks / window.Seconds() * 100

modules/disk-load/cmd/disk-load/cpu_test.go

@@ -191,16 +191,19 @@ func TestCPUTopRanksAUnitsProcesses(t *testing.T) {
 		f.then("/proc/"+itoa(pid)+"/stat", itoa(pid)+" ("+comm+") S 1 1 1 0 -1 0 0 0 0 0 "+itoa(ticks)+" 0 0 0 20 0 "+itoa(threads)+" 0 1 1 1\n")
 	}
 	for pid, c := range map[int][2]string{10: {"firefox", "1"}, 11: {"sh (x)", "1"}, 12: {"gone", "1"}} {
-		_ = c
 		f.file("/proc/"+itoa(pid)+"/stat", itoa(pid)+" ("+c[0]+") S 1 1 1 0 -1 0 0 0 0 0 0 0 0 0 20 0 "+c[1]+" 0 1 1 1\n")
 	}
 	f.file("/proc/13/stat", "13 (deep) S 1 1 1 0 -1 0 0 0 0 0 0 0 0 0 20 0 4 0 1 1 1\n")
 	f.file("/proc/10/cmdline", "/usr/bin/firefox\x00--new-window\x00")
+	f.file("/proc/11/cmdline", "mysql\x00--password\x00hunter2\x00")
 	st(10, "firefox", 150, 40) // 75 % of one CPU over 2 s
 	st(11, "sh (x)", 40, 1)    // 20 %
 	st(13, "deep", 10, 4)      // 5 %, in the child cgroup
 	f.then("/sys/fs/cgroup/user.slice/session-c1.scope/cgroup.procs", "10\n11\n14\n")
-	st(14, "newborn", 500, 1) // not in the first reading: skipped
+	f.file("/sys/fs/cgroup/user.slice/session-c1.scope/app/cgroup.procs", "13\n15\n")
+	f.file("/proc/15/stat", "15 (old) S 1 1 1 0 -1 0 0 0 0 0 0 0 0 0 20 0 1 0 1 1 1\n")
+	f.then("/proc/15/stat", "15 (reused) S 1 1 1 0 -1 0 0 0 0 0 900 0 0 0 20 0 1 0 77 1 1\n") // new starttime: another process
+	st(14, "newborn", 500, 1)                                                                 // not in the first reading: skipped
 
 	out, err := tool(t, f.machine(), "cpu_top").Run(map[string]any{"unit": "session-c1.scope", "seconds": 2.0, "limit": 3.0})
 	if err != nil {
@@ -208,9 +211,19 @@ func TestCPUTopRanksAUnitsProcesses(t *testing.T) {
 	}
 	top := out.(map[string]any)["top"].([]ProcessCPU)
 	if len(top) != 3 || top[2].PID != 13 || top[2].CPUPct != 5 || top[0].PID != 10 || top[0].CPUPct != 75 || top[0].SharePct != 75 || top[0].Threads != 40 ||
-		top[0].Command != "firefox: /usr/bin/firefox --new-window" || top[1].PID != 11 || top[1].CPUPct != 20 || top[1].Command != "sh (x)" {
+		top[0].Command != "firefox: /usr/bin/firefox" || top[1].PID != 11 || top[1].CPUPct != 20 || top[1].Command != "sh (x): mysql" {
 		t.Errorf("processes of the unit: %+v", top)
 	}
+	for _, p := range top {
+		if p.PID == 15 || strings.Contains(p.Command, "hunter2") {
+			t.Errorf("a reused pid was counted, or an argument leaked: %+v", p)
+		}
+	}
+	for _, u := range []string{"../../etc", "/sys/fs/cgroup/../../proc", "user.slice/../.."} {
+		if _, err := tool(t, f.machine(), "cpu_top").Run(map[string]any{"unit": u}); err == nil {
+			t.Errorf("cgroup path %q escaping the root was accepted", u)
+		}
+	}
 	if _, err := tool(t, f.machine(), "cpu_top").Run(map[string]any{"unit": "nothing.scope"}); err == nil {
 		t.Error("an unknown unit was accepted")
 	}