fix(evt): some correctness changes around cwd/perms/timestamps (#127)

Author: bleggett

rust-toolchain.toml

@@ -0,0 +1,3 @@
+[toolchain]
+channel = "1.95.0"
+components = ["rustfmt", "rust-std", "clippy"]

src/base_plugin/mod.rs

@@ -319,39 +319,19 @@ impl EderaPlugin {
     }
 
     pub fn extract_is_open_read(&mut self, mut req: ExtractRequest<Self>) -> Result<bool> {
-        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| {
-            if let Ok(res) = parsers::get_openstate(zone_evt) {
-                return Ok(res == parsers::OpenType::Read);
-            }
-            Ok(false)
-        })
+        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| Ok(parsers::is_open_read(zone_evt)))
     }
 
     pub fn extract_is_open_write(&mut self, mut req: ExtractRequest<Self>) -> Result<bool> {
-        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| {
-            if let Ok(res) = parsers::get_openstate(zone_evt) {
-                return Ok(res == parsers::OpenType::Write);
-            }
-            Ok(false)
-        })
+        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| Ok(parsers::is_open_write(zone_evt)))
     }
 
     pub fn extract_is_open_exec(&mut self, mut req: ExtractRequest<Self>) -> Result<bool> {
-        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| {
-            if let Ok(res) = parsers::get_openstate(zone_evt) {
-                return Ok(res == parsers::OpenType::Exec);
-            }
-            Ok(false)
-        })
+        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| Ok(parsers::is_open_exec(zone_evt)))
     }
 
     pub fn extract_is_open_create(&mut self, mut req: ExtractRequest<Self>) -> Result<bool> {
-        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| {
-            if let Ok(res) = parsers::get_openstate(zone_evt) {
-                return Ok(res == parsers::OpenType::Create);
-            }
-            Ok(false)
-        })
+        self.with_zone_syscall_evt_ctx(&mut req, |zone_evt| Ok(parsers::is_open_create(zone_evt)))
     }
 
     pub fn extract_count(&mut self, _: ExtractRequest<Self>) -> Result<u64> {
@@ -935,7 +915,7 @@ impl EderaPlugin {
                 self.threadstate
                     .with_threadinfo(&evt.zone_id, &evt.thread_id, |tinfo| {
                         if tinfo.clone_ts != 0 {
-                            Some(evt.timestamp - tinfo.clone_ts)
+                            Some(evt.timestamp.saturating_sub(tinfo.clone_ts))
                         } else {
                             None
                         }
@@ -1154,7 +1134,11 @@ impl EderaPlugin {
                         if evt.event_type == event_codes::PPME_SCHEDSWITCH_1_E as u32
                             || evt.event_type == event_codes::PPME_SCHEDSWITCH_6_E as u32
                         {
-                            Some(exec_time.last_switch_ts - exec_time.previous_switch_ts)
+                            Some(
+                                exec_time
+                                    .last_switch_ts
+                                    .saturating_sub(exec_time.previous_switch_ts),
+                            )
                         } else {
                             // TODO(bml) libsinsp only reports this for explicit switch events,
                             // we could actually do better, but for now maintain strict compat
@@ -1207,12 +1191,12 @@ impl EderaPlugin {
                     .filter(|s| !s.is_empty())
                     .find_map(|entry| {
                         // Each entry is "subsystem=path"
-                        if let Some((name, path)) = entry.split_once('=')
-                            && (name == subsystem || name == format!("{}_cgroup", subsystem))
-                        {
-                            CString::new(path.to_string()).ok();
+                        let (name, path) = entry.split_once('=')?;
+                        if name == subsystem || name == format!("{}_cgroup", subsystem) {
+                            CString::new(path.to_string()).ok()
+                        } else {
+                            None
                         }
-                        None
                     })
             })
             .unwrap_or(CString::new("NA").expect("default value must parse")))
@@ -2422,7 +2406,7 @@ impl EderaPlugin {
             .and_then(|evt| {
                 self.with_nth_parent_proc_thread(&evt.zone_id, &evt.thread_id, 1, |atinfo| {
                     if atinfo.clone_ts != 0 {
-                        Some(evt.timestamp - atinfo.clone_ts)
+                        Some(evt.timestamp.saturating_sub(atinfo.clone_ts))
                     } else {
                         None
                     }

src/parsers.rs

@@ -21,53 +21,83 @@ const FLAGS_SOCKET_CONNECTED: u32 = 1 << 13;
 const FLAGS_OVERLAY_UPPER: u32 = 1 << 17;
 const FLAGS_OVERLAY_LOWER: u32 = 1 << 18;
 
-#[derive(PartialEq)]
-pub enum OpenType {
-    Read,
-    Write,
-    Exec,
-    Create,
-}
-
-pub fn get_openstate(evt: &ZoneKernelSyscallEvent) -> Result<OpenType> {
-    // strum from discriminant to make this simpler
-    let etype =
-        event_codes::from_repr(evt.event_type).ok_or(anyhow!("could not parse event type"))?;
-
-    if is_open_file(etype) {
-        let is_new_version = etype == event_codes::PPME_SYSCALL_OPENAT_2_X
-            || etype == event_codes::PPME_SYSCALL_OPENAT2_X;
-        // new versions have open flags at arg 3 insted of arg 2
-        let flags = if is_new_version {
-            u32::from_ne_bytes(evt.event_params[3].param_data.as_slice().try_into()?)
-        } else {
-            u32::from_ne_bytes(evt.event_params[2].param_data.as_slice().try_into()?)
-        };
+// Open access/create/exec classification mirrors libsinsp
+// `evt.is_open_read/write/exec/create`.
+const OPEN_EXEC_MODE_MASK: u32 =
+    ppm_consts::PPM_S_IXUSR | ppm_consts::PPM_S_IXGRP | ppm_consts::PPM_S_IXOTH;
+
+fn read_u32_param(evt: &ZoneKernelSyscallEvent, idx: usize) -> Option<u32> {
+    let param = evt.event_params.get(idx)?;
+    Some(u32::from_ne_bytes(
+        param.param_data.as_slice().try_into().ok()?,
+    ))
+}
 
-        if (flags & ppm_consts::PPM_O_RDONLY) != 0 {
-            return Ok(OpenType::Read);
-        } else if (flags & ppm_consts::PPM_O_WRONLY) != 0 {
-            return Ok(OpenType::Write);
-        } else if (flags & ppm_consts::PPM_O_F_CREATED) != 0 {
-            return Ok(OpenType::Create);
-        } else if (flags & (ppm_consts::PPM_O_TMPFILE | ppm_consts::PPM_O_CREAT)) != 0
-            && etype != event_codes::PPME_SYSCALL_OPEN_BY_HANDLE_AT_X
-        {
-            let mode_bits = if is_new_version {
-                u32::from_ne_bytes(evt.event_params[4].param_data.as_slice().try_into()?)
-            } else {
-                u32::from_ne_bytes(evt.event_params[3].param_data.as_slice().try_into()?)
+/// Normalized open flags for an open-family exit event (the `flags` param), or
+/// `None` if this isn't such an event or the param is absent/malformed. The
+/// modern openat variants carry flags at arg 3, the legacy forms at arg 2.
+fn open_flags(evt: &ZoneKernelSyscallEvent, etype: event_codes) -> Option<u32> {
+    let idx = match etype {
+        event_codes::PPME_SYSCALL_OPENAT_2_X | event_codes::PPME_SYSCALL_OPENAT2_X => 3,
+        event_codes::PPME_SYSCALL_OPEN_X | event_codes::PPME_SYSCALL_OPEN_BY_HANDLE_AT_X => 2,
+        _ => return None,
+    };
+    read_u32_param(evt, idx)
+}
+
+pub fn is_open_read(evt: &ZoneKernelSyscallEvent) -> bool {
+    let Some(etype) = event_codes::from_repr(evt.event_type) else {
+        return false;
+    };
+    open_flags(evt, etype).is_some_and(|flags| (flags & ppm_consts::PPM_O_RDONLY) != 0)
+}
+
+pub fn is_open_write(evt: &ZoneKernelSyscallEvent) -> bool {
+    let Some(etype) = event_codes::from_repr(evt.event_type) else {
+        return false;
+    };
+    open_flags(evt, etype).is_some_and(|flags| (flags & ppm_consts::PPM_O_WRONLY) != 0)
+}
+
+pub fn is_open_create(evt: &ZoneKernelSyscallEvent) -> bool {
+    let Some(etype) = event_codes::from_repr(evt.event_type) else {
+        return false;
+    };
+    let Some(flags) = open_flags(evt, etype) else {
+        return false;
+    };
+    // O_F_CREATED means the file was created; O_TMPFILE creates one only on success.
+    (flags & ppm_consts::PPM_O_F_CREATED) != 0
+        || ((flags & ppm_consts::PPM_O_TMPFILE) != 0 && get_retval(evt).is_some_and(|r| r >= 0))
+}
+
+pub fn is_open_exec(evt: &ZoneKernelSyscallEvent) -> bool {
+    let Some(etype) = event_codes::from_repr(evt.event_type) else {
+        return false;
+    };
+    // `creat` carries mode at arg 2; open-family carries it at arg 3 (legacy) or
+    // arg 4 (modern), and only counts as exec when the open can create the file.
+    // open_by_handle_at has no mode param and is excluded, as in libsinsp.
+    let mode_idx = match etype {
+        event_codes::PPME_SYSCALL_CREAT_X => 2,
+        event_codes::PPME_SYSCALL_OPEN_X
+        | event_codes::PPME_SYSCALL_OPENAT_2_X
+        | event_codes::PPME_SYSCALL_OPENAT2_X => {
+            let Some(flags) = open_flags(evt, etype) else {
+                return false;
             };
-            if (mode_bits
-                & (ppm_consts::PPM_S_IXUSR | ppm_consts::PPM_S_IXGRP | ppm_consts::PPM_S_IXOTH))
-                != 0
-            {
-                return Ok(OpenType::Exec);
+            if (flags & (ppm_consts::PPM_O_TMPFILE | ppm_consts::PPM_O_CREAT)) == 0 {
+                return false;
+            }
+            if etype == event_codes::PPME_SYSCALL_OPEN_X {
+                3
+            } else {
+                4
             }
         }
-    }
-
-    Err(anyhow!("not an open event"))
+        _ => return false,
+    };
+    read_u32_param(evt, mode_idx).is_some_and(|mode| (mode & OPEN_EXEC_MODE_MASK) != 0)
 }
 
 pub fn is_enter(evt: &ZoneKernelSyscallEvent) -> bool {
@@ -436,4 +466,69 @@ mod tests {
         assert_eq!(get_retval(&evt), None);
         assert_eq!(syscall_failed(&evt), None);
     }
+
+    // constructs a modern openat exit event: params are [fd, dirfd, name, flags, mode].
+    // fd=3 (success) so the O_TMPFILE create path is satisfied when exercised.
+    fn openat2x_event(flags: u32, mode: u32) -> ZoneKernelSyscallEvent {
+        let u32_param = |v: u32| ZoneKernelEventParam {
+            param_data: v.to_ne_bytes().to_vec(),
+            ..Default::default()
+        };
+        ZoneKernelSyscallEvent {
+            event_type: event_codes::PPME_SYSCALL_OPENAT_2_X as u32,
+            event_category: "EC_FILE | EC_SYSCALL".to_string(),
+            event_params: vec![
+                u32_param(3),
+                u32_param(0),
+                u32_param(0),
+                u32_param(flags),
+                u32_param(mode),
+            ],
+            ..Default::default()
+        }
+    }
+
+    #[test]
+    fn rdonly_open_is_read_only() {
+        let evt = openat2x_event(ppm_consts::PPM_O_RDONLY, 0);
+        assert!(is_open_read(&evt));
+        assert!(!is_open_write(&evt));
+    }
+
+    #[test]
+    fn wronly_open_is_write_only() {
+        let evt = openat2x_event(ppm_consts::PPM_O_WRONLY, 0);
+        assert!(!is_open_read(&evt));
+        assert!(is_open_write(&evt));
+    }
+
+    #[test]
+    fn rdwr_open_is_both_read_and_write() {
+        let evt = openat2x_event(ppm_consts::PPM_O_RDWR, 0);
+        assert!(is_open_read(&evt));
+        assert!(is_open_write(&evt));
+    }
+
+    #[test]
+    fn created_flag_is_create_independent_of_access_mode() {
+        let evt = openat2x_event(ppm_consts::PPM_O_WRONLY | ppm_consts::PPM_O_F_CREATED, 0);
+        assert!(is_open_create(&evt));
+        assert!(is_open_write(&evt));
+    }
+
+    #[test]
+    fn creating_open_with_exec_mode_is_exec() {
+        let evt = openat2x_event(
+            ppm_consts::PPM_O_WRONLY | ppm_consts::PPM_O_CREAT,
+            ppm_consts::PPM_S_IXUSR,
+        );
+        assert!(is_open_exec(&evt));
+    }
+
+    #[test]
+    fn plain_read_open_is_neither_create_nor_exec() {
+        let evt = openat2x_event(ppm_consts::PPM_O_RDONLY, 0);
+        assert!(!is_open_create(&evt));
+        assert!(!is_open_exec(&evt));
+    }
 }

src/threadstate.rs

@@ -2089,6 +2089,12 @@ impl ZoneInfo {
     }
 
     fn parse_chdir_exit(&mut self, event: &ZoneKernelSyscallEvent) -> Result<()> {
+        // if syscall failed, just bail, we didn't actually change the cwd
+        if parsers::get_retval(event).filter(|&v| v >= 0).is_none() {
+            debug!("no success retval found for chdir event: {:?}", event);
+            return Ok(());
+        };
+
         // if we have a thread for this event, update the cwd of that thread, otherwise NBD
         self.with_mut_threadinfo_ctx(event, |tinfo| {
             tinfo.cwd = event.event_params[1].param_pretty.clone(); // pretty version is already a string
@@ -2449,8 +2455,9 @@ impl ZoneInfo {
             ExecTime {
                 last_switch_ts: event.timestamp,
                 previous_switch_ts: exectime.last_switch_ts,
-                cumulative_switch_time: exectime.cumulative_switch_time
-                    + (event.timestamp - exectime.last_switch_ts),
+                cumulative_switch_time: exectime
+                    .cumulative_switch_time
+                    .saturating_add(event.timestamp.saturating_sub(exectime.last_switch_ts)),
             },
         );