learn: follow-ups from the #209 review

crates/sandlock-cli/src/learn.rs

@@ -4,7 +4,7 @@
 //! usable by `sandlock run -p`.
 
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 
 use anyhow::{anyhow, Result};
@@ -76,7 +76,7 @@ fn collapse_write_paths(writes: &BTreeSet<PathBuf>) -> Vec<PathBuf> {
         if is_junk_path(p) { continue; }
         let p = &fold_session_path(p.clone());
         if p.exists() {
-            if p.as_os_str().as_encoded_bytes() == b"/" {
+            if is_fs_root(p) {
                 eprintln!(
                     "sandlock learn: WARNING: observed a direct write of '/', refusing to grant it"
                 );
@@ -95,7 +95,7 @@ fn collapse_write_paths(writes: &BTreeSet<PathBuf>) -> Vec<PathBuf> {
             continue;
         }
         let Some(ancestor) = p.ancestors().skip(1).find(|a| a.exists()) else { continue };
-        if ancestor.as_os_str().as_encoded_bytes() == b"/" {
+        if is_fs_root(&ancestor) {
             eprintln!(
                 "sandlock learn: WARNING: write collapse for '{}' reaches filesystem root, skipping",
                 p.display()
@@ -200,6 +200,10 @@ fn collapse_by_threshold(
     out.into_iter().collect()
 }
 
+fn is_fs_root(p: &Path) -> bool {
+    p == Path::new("/")
+}
+
 /// Returns true for pid-specific paths that are meaningless across runs.
 fn is_junk_path(p: &std::path::Path) -> bool {
     let b = p.as_os_str().as_encoded_bytes();
@@ -406,7 +410,7 @@ impl LearnObserver {
             // only the parent is canonicalized to avoid following the symlink.
             "unlinkat" | "symlinkat" => {
                 if let Some(p) = event.path {
-                    let p = canonicalize_parent_or_keep(p);
+                    let p = canonicalize_parent_or_keep(p, event.pid);
                     if let Some(parent) = p.parent() {
                         self.writes.lock().unwrap().insert(parent.to_path_buf());
                     }
@@ -416,7 +420,7 @@ impl LearnObserver {
             // Both operate on the link itself, not its target.
             "renameat2" => {
                 for p in [event.path, event.path2].into_iter().flatten() {
-                    let p = canonicalize_parent_or_keep(p);
+                    let p = canonicalize_parent_or_keep(p, event.pid);
                     if let Some(parent) = p.parent() {
                         self.writes.lock().unwrap().insert(parent.to_path_buf());
                     }
@@ -429,7 +433,7 @@ impl LearnObserver {
                     self.reads.lock().unwrap().insert(canonicalize_or_keep(src, event.pid));
                 }
                 if let Some(dst) = event.path2 {
-                    let dst = canonicalize_parent_or_keep(dst);
+                    let dst = canonicalize_parent_or_keep(dst, event.pid);
                     if let Some(parent) = dst.parent() {
                         self.writes.lock().unwrap().insert(parent.to_path_buf());
                     }
@@ -517,27 +521,30 @@ impl LearnObserver {
 ///
 /// /proc/self paths are canonicalized via the event pid so symlinks
 /// (e.g. /proc/self/exe) resolve against the workload, not the supervisor.
-/// Results still under /proc/<pid>/ are mapped back to /proc/self/.
+/// Results still under /proc/<pid> are mapped back to /proc/self.
 fn canonicalize_or_keep(p: PathBuf, pid: u32) -> PathBuf {
-    let b = p.as_os_str().as_encoded_bytes();
-    if b.starts_with(b"/proc/self") {
-        let suffix = &b[b"/proc/self".len()..];
-        let pid_path = PathBuf::from(format!("/proc/{}{}", pid, String::from_utf8_lossy(suffix)));
-        let resolved = std::fs::canonicalize(&pid_path).unwrap_or(pid_path);
-        let resolved_b = resolved.as_os_str().as_encoded_bytes();
-        let pid_prefix = format!("/proc/{}/", pid);
-        if resolved_b.starts_with(pid_prefix.as_bytes()) {
-            let rest = &resolved_b[pid_prefix.len() - 1..]; // keep leading /
-            return PathBuf::from(format!("/proc/self{}", String::from_utf8_lossy(rest)));
-        }
-        return resolved;
+    let proc_self = Path::new("/proc/self");
+    let Ok(rest) = p.strip_prefix(proc_self) else {
+        return std::fs::canonicalize(&p).unwrap_or(p);
+    };
+    let proc_pid = Path::new("/proc").join(pid.to_string());
+    let pid_path = join_rest(&proc_pid, rest);
+    let resolved = std::fs::canonicalize(&pid_path).unwrap_or(pid_path);
+    match resolved.strip_prefix(&proc_pid) {
+        Ok(rest) => join_rest(proc_self, rest),
+        Err(_) => resolved,
     }
-    std::fs::canonicalize(&p).unwrap_or(p)
+}
+
+/// PathBuf::join appends a separator even for an empty component, which
+/// would turn the bare directory into "dir/".
+fn join_rest(base: &Path, rest: &Path) -> PathBuf {
+    if rest.as_os_str().is_empty() { base.to_path_buf() } else { base.join(rest) }
 }
 
 /// Canonicalize only the parent directory and rejoin the final component.
 /// Used for syscalls that operate on the link itself (unlink, rename, symlink).
-fn canonicalize_parent_or_keep(p: PathBuf) -> PathBuf {
+fn canonicalize_parent_or_keep(p: PathBuf, pid: u32) -> PathBuf {
     let file_name = match p.file_name() {
         Some(n) => n.to_owned(),
         None => return p,
@@ -546,8 +553,7 @@ fn canonicalize_parent_or_keep(p: PathBuf) -> PathBuf {
         Some(par) => par,
         None => return p,
     };
-    let canonical_parent = std::fs::canonicalize(parent).unwrap_or_else(|_| parent.to_path_buf());
-    canonical_parent.join(file_name)
+    canonicalize_or_keep(parent.to_path_buf(), pid).join(file_name)
 }
 
 pub async fn run(args: LearnArgs) -> Result<()> {
@@ -689,7 +695,7 @@ pub async fn run(args: LearnArgs) -> Result<()> {
             // whose cwd is the workdir root lists it routinely (python's -c
             // puts the cwd on sys.path), and granting it would subsume every
             // other read in the profile.
-            if p.as_path() == std::path::Path::new("/") {
+            if is_fs_root(p) {
                 eprintln!(
                     "sandlock learn: WARNING: observed a direct read of '/', refusing to grant it"
                 );

crates/sandlock-cli/tests/learn_test.rs

@@ -822,6 +822,31 @@ fn test_proc_self_exe_resolves_to_binary() {
     let read_line = stdout.lines().find(|l| l.starts_with("read = [")).unwrap_or("");
     assert!(!read_line.contains("\"/proc/self/exe\""),
         "/proc/self/exe must be resolved to the binary path, not recorded as-is: {read_line}");
+    let interp = std::process::Command::new("python3")
+        .args(["-c", "import sys; print(sys.executable)"])
+        .output()
+        .expect("run python3");
+    let interp = String::from_utf8_lossy(&interp.stdout).trim().to_string();
+    let interp = std::fs::canonicalize(&interp).expect("canonicalize python3");
+    let expected = format!("\"{}\"", interp.display());
+    assert!(read_line.contains(&expected),
+        "expected resolved interpreter {expected} in reads, got: {read_line}");
+}
+
+/// Opening /proc/self itself (no sub-entry) must be recorded as /proc/self,
+/// not resolved to /proc/<pid> and then dropped as junk.
+#[test]
+fn test_proc_self_bare_preserved() {
+    let output = sandlock_bin()
+        .args(["learn", "--", "python3", "-c", "import os; os.listdir('/proc/self')"])
+        .output()
+        .expect("failed to run sandlock learn");
+    assert!(output.status.success(),
+        "sandlock learn failed: stderr={}", String::from_utf8_lossy(&output.stderr));
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    let read_line = stdout.lines().find(|l| l.starts_with("read = [")).unwrap_or("");
+    assert!(read_line.contains("\"/proc/self\""),
+        "/proc/self must be recorded as-is, got: {read_line}");
 }
 
 // ── Merge and canonicalization ────────────────────────────────────────────────

docs/learn.md

@@ -59,7 +59,7 @@ optional, so omitting the ancestor would cause `sandlock run` to abort.
 | **Guarded** | `$HOME`, `/etc`, `/proc`, `/sys`, `/dev`, `/boot`, `/run/secrets` | emit + warning + diff | never (keep individual files; override with `--force-sensitive-collapse`) |
 | **Normal** | everything else | collapse freely | collapse freely |
 
-The tiers apply to write collapse only. **The only path dropped from direct writes/reads is `/`**: granting it would subsume every other entry in the profile. Also, warning is printed to stderr when the direct write path is Protected or Guarded. 
+The tiers apply to write collapse only. **The only path dropped from direct writes and reads is `/`**: granting it would subsume every other entry in the profile. A direct write to a Protected or Guarded path is still recorded, with a NOTE printed to stderr.
 
 When a write collapse lands on a guarded path, a warning is printed to
 stderr along with an **observed-vs-granted diff**, the list of siblings