Skip to content

Commit 7e4a342

Browse files
authored
Re-introduce forked but not exec'd heuristic (#1415)
### What does this PR do? This commit re-introduces the forked but not exec'd heuristic accidentally removed in PR #1322. This commit re-introduces this heuristic, extracting it into a function and adding tests to hopefully avoid this situation in the future. Once merged we will cut a new version of lading.
1 parent 1c4e868 commit 7e4a342

File tree

2 files changed

+204
-4
lines changed

2 files changed

+204
-4
lines changed

CLAUDE.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,11 @@ ALWAYS prefer property tests over unit tests. Unit tests are insufficient for
9898
lading's requirements. We use [proptest](https://github.com/proptest-rs/proptest)
9999
for property testing.
100100

101+
Test naming conventions:
102+
- Don't prefix test names with `test_` - they're obviously tests
103+
- Don't prefix property test generators with `prop_` - they're obviously property tests
104+
- Use descriptive names that explain what property or behavior is being tested
105+
101106
Critical components are those which must function correctly or lading itself
102107
cannot function. These require proofs using [kani](https://github.com/model-checking/kani):
103108
- Throttling (MUST be correct - lading is used to make throughput claims)

lading/src/observer/linux/procfs.rs

Lines changed: 199 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ mod uptime;
55

66
use std::io;
77

8-
use metrics::gauge;
8+
use metrics::{counter, gauge};
99
use nix::errno::Errno;
1010
use procfs::process::Process;
1111
use rustc_hash::FxHashMap;
@@ -15,6 +15,25 @@ use crate::observer::linux::utils::process_descendents::ProcessDescendantsIterat
1515

1616
const BYTES_PER_KIBIBYTE: u64 = 1024;
1717

18+
/// Determines if a child process is forked but not exec'd by comparing with its
19+
/// parent.
20+
///
21+
/// When a process forks, the child initially shares memory with the parent
22+
/// until it calls `exec` to replace its memory space with a new program. During
23+
/// this fork-but-not-exec state, both processes have identical exe paths and
24+
/// command lines.
25+
///
26+
/// This heuristic is critical for accurate memory accounting in lading. Without
27+
/// it, we double-count memory usage because the forked child and its parent:
28+
/// the child appears to have its own memory in `/proc/<pid>/smaps` but it's
29+
/// actually sharing pages with the parent.
30+
///
31+
/// Returns true if the child is forked but not exec'd, false otherwise.
32+
#[inline]
33+
fn forked_but_not_execd(child: &ProcessInfo, parent: &ProcessInfo) -> bool {
34+
child.exe == parent.exe && child.cmdline == parent.cmdline
35+
}
36+
1837
#[derive(thiserror::Error, Debug)]
1938
/// Errors produced by functions in this module
2039
pub enum Error {
@@ -91,7 +110,8 @@ impl Sampler {
91110
self.process_info.clear();
92111

93112
for process in ProcessDescendantsIterator::new(self.parent.pid) {
94-
let process_info = match initialize_process_info(process.pid()).await {
113+
let pid = process.pid();
114+
let process_info = match initialize_process_info(pid).await {
95115
Ok(Some(info)) => info,
96116
Ok(None) => {
97117
warn!("Could not initialize process info, will retry.");
@@ -102,10 +122,21 @@ impl Sampler {
102122
return Ok(());
103123
}
104124
};
105-
self.process_info.insert(process.pid(), process_info);
125+
126+
if let Ok(stat) = process.stat() {
127+
let parent_pid = stat.ppid;
128+
if let Some(parent_info) = self.process_info.get(&parent_pid) {
129+
if forked_but_not_execd(&process_info, parent_info) {
130+
counter!("process_skipped").increment(1);
131+
processes_skipped += 1;
132+
continue;
133+
}
134+
}
135+
}
136+
137+
self.process_info.insert(pid, process_info);
106138

107139
processes_found += 1;
108-
let pid = process.pid();
109140
match self.handle_process(process, &mut aggr, include_smaps).await {
110141
Ok(true) => {
111142
// handled successfully
@@ -365,3 +396,167 @@ async fn proc_cmdline(pid: i32) -> Result<String, Error> {
365396
};
366397
Ok(res)
367398
}
399+
400+
#[cfg(test)]
401+
mod tests {
402+
use super::*;
403+
use proptest::prelude::*;
404+
405+
prop_compose! {
406+
/// Generate a valid executable path
407+
fn arb_exe_path()(
408+
components in prop::collection::vec("[a-zA-Z0-9_-]+", 1..5),
409+
) -> String {
410+
format!("/usr/bin/{}", components.join("/"))
411+
}
412+
}
413+
414+
prop_compose! {
415+
/// Generate a command line with arguments
416+
fn arb_cmdline()(
417+
cmd in "[a-zA-Z0-9_-]+",
418+
args in prop::collection::vec("[a-zA-Z0-9_=-]+", 0..5),
419+
) -> String {
420+
if args.is_empty() {
421+
cmd
422+
} else {
423+
format!("{} {}", cmd, args.join(" "))
424+
}
425+
}
426+
}
427+
428+
prop_compose! {
429+
/// Generate process info for testing
430+
fn arb_process_info()(
431+
exe in arb_exe_path(),
432+
cmdline in arb_cmdline(),
433+
comm in "[a-zA-Z0-9_-]+",
434+
pid in 1..100000i32,
435+
) -> ProcessInfo {
436+
ProcessInfo {
437+
exe,
438+
cmdline,
439+
comm,
440+
pid_s: pid.to_string(),
441+
stat_sampler: stat::Sampler::new(),
442+
}
443+
}
444+
}
445+
446+
proptest! {
447+
#[test]
448+
fn identical_processes_are_forked_but_not_execed(
449+
exe in arb_exe_path(),
450+
cmdline in arb_cmdline(),
451+
comm1 in "[a-zA-Z0-9_-]+",
452+
comm2 in "[a-zA-Z0-9_-]+",
453+
pid1 in 1..100000i32,
454+
pid2 in 1..100000i32,
455+
) {
456+
let parent = ProcessInfo {
457+
exe: exe.clone(),
458+
cmdline: cmdline.clone(),
459+
comm: comm1,
460+
pid_s: pid1.to_string(),
461+
stat_sampler: stat::Sampler::new(),
462+
};
463+
let child = ProcessInfo {
464+
exe,
465+
cmdline,
466+
comm: comm2,
467+
pid_s: pid2.to_string(),
468+
stat_sampler: stat::Sampler::new(),
469+
};
470+
471+
assert!(forked_but_not_execd(&child, &parent),
472+
"Processes with identical exe and cmdline should be detected as forked-but-not-execed");
473+
}
474+
475+
#[test]
476+
fn different_exe_means_execed(
477+
parent in arb_process_info(),
478+
mut child in arb_process_info(),
479+
) {
480+
// Ensure child has different exe
481+
child.exe = format!("{}_different", parent.exe);
482+
483+
assert!(!forked_but_not_execd(&child, &parent),
484+
"Processes with different exe paths should NOT be detected as forked-but-not-execed");
485+
}
486+
487+
#[test]
488+
fn different_cmdline_means_execed(
489+
parent in arb_process_info(),
490+
mut child in arb_process_info(),
491+
) {
492+
// Ensure child has same exe but different cmdline
493+
child.exe = parent.exe.clone();
494+
child.cmdline = format!("{} --extra-arg", parent.cmdline);
495+
496+
assert!(!forked_but_not_execd(&child, &parent),
497+
"Processes with different cmdlines should NOT be detected as forked-but-not-execed");
498+
}
499+
500+
#[test]
501+
fn empty_strings_handled_correctly(
502+
has_exe in prop::bool::ANY,
503+
has_cmdline in prop::bool::ANY,
504+
) {
505+
let parent = ProcessInfo {
506+
exe: if has_exe { "/bin/test".to_string() } else { String::new() },
507+
cmdline: if has_cmdline { "test arg".to_string() } else { String::new() },
508+
comm: "test".to_string(),
509+
pid_s: "1".to_string(),
510+
stat_sampler: stat::Sampler::new(),
511+
};
512+
let child = ProcessInfo {
513+
exe: if has_exe { "/bin/test".to_string() } else { String::new() },
514+
cmdline: if has_cmdline { "test arg".to_string() } else { String::new() },
515+
comm: "test".to_string(),
516+
pid_s: "2".to_string(),
517+
stat_sampler: stat::Sampler::new(),
518+
};
519+
520+
// Both have same exe and cmdline (even if empty), so should be detected
521+
assert!(forked_but_not_execd(&child, &parent));
522+
}
523+
524+
#[test]
525+
fn whitespace_sensitivity(
526+
base_cmdline in arb_cmdline(),
527+
extra_spaces in prop::collection::vec(prop::bool::ANY, 0..3),
528+
) {
529+
let parent = ProcessInfo {
530+
exe: "/bin/test".to_string(),
531+
cmdline: base_cmdline.clone(),
532+
comm: "test".to_string(),
533+
pid_s: "1".to_string(),
534+
stat_sampler: stat::Sampler::new(),
535+
};
536+
537+
// Add extra spaces based on the bool vector
538+
let mut modified_cmdline = base_cmdline.clone();
539+
for add_space in extra_spaces {
540+
if add_space {
541+
modified_cmdline.push(' ');
542+
}
543+
}
544+
545+
let child = ProcessInfo {
546+
exe: "/bin/test".to_string(),
547+
cmdline: modified_cmdline.clone(),
548+
comm: "test".to_string(),
549+
pid_s: "2".to_string(),
550+
stat_sampler: stat::Sampler::new(),
551+
};
552+
553+
// If cmdlines differ (even by whitespace), should NOT be detected as forked-but-not-execed
554+
if base_cmdline != modified_cmdline {
555+
assert!(!forked_but_not_execd(&child, &parent),
556+
"Even whitespace differences should mean the process has exec'd");
557+
} else {
558+
assert!(forked_but_not_execd(&child, &parent));
559+
}
560+
}
561+
}
562+
}

0 commit comments

Comments
 (0)