Add snapshot test support for coverage implementation (#3368)

Add a snapshot-based test to our coverage implementation. This adds coverage for a bunch of code which did not previously have it, and allows us to easily see that coverage is being generated as expected.

A fix was made to the recording code to remove the use of `TargetAllowList` as it was easy to misconfigure `CoverageRecorder` when using it. The use of `TargetAllowList` is now only a container struct in our generic coverage task.

src/agent/Cargo.lock

@@ -590,12 +590,16 @@ name = "coverage"
 version = "0.1.0"
 dependencies = [
  "anyhow",
+ "cc",
  "clap",
  "cobertura",
+ "coverage",
  "debuggable-module",
  "debugger",
+ "dunce",
  "env_logger",
  "iced-x86",
+ "insta",
  "log",
  "nix",
  "pete",
@@ -603,6 +607,7 @@ dependencies = [
  "procfs",
  "regex",
  "symbolic",
+ "tempfile",
  "thiserror",
 ]
 

src/agent/coverage/Cargo.toml

@@ -4,6 +4,9 @@ version = "0.1.0"
 edition = "2021"
 license = "MIT"
 
+[features]
+slow-tests = []
+
 [dependencies]
 anyhow = { version = "1.0", features = ["backtrace"] }
 cobertura = { path = "../cobertura" }
@@ -32,3 +35,8 @@ procfs = { version = "0.12", default-features = false, features = ["flate2"] }
 clap = { version = "4.3", features = ["derive"] }
 env_logger = "0.10.0"
 pretty_assertions = "1.4.0"
+insta = { version = "1.31.0", features = ["glob"] }
+coverage = { path = "../coverage" }
+cc = "1.0"
+tempfile = "3.7.0"
+dunce = "1.0"

src/agent/coverage/examples/record.rs

@@ -2,10 +2,10 @@ use std::process::Command;
 use std::sync::Arc;
 use std::time::Duration;
 
-use anyhow::{bail, Result};
+use anyhow::{bail, Context, Result};
 use clap::Parser;
 use cobertura::CoberturaCoverage;
-use coverage::allowlist::{AllowList, TargetAllowList};
+use coverage::allowlist::AllowList;
 use coverage::binary::{BinaryCoverage, DebugInfoCache};
 use coverage::record::{CoverageRecorder, Recorded};
 use debuggable_module::load_module::LoadModule;
@@ -57,19 +57,21 @@ fn main() -> Result<()> {
         .map(Duration::from_millis)
         .unwrap_or(DEFAULT_TIMEOUT);
 
-    let mut allowlist = TargetAllowList::default();
+    let module_allowlist = args
+        .module_allowlist
+        .map(AllowList::load)
+        .unwrap_or_else(|| Ok(AllowList::default()))
+        .context("loading module allowlist")?;
 
-    if let Some(path) = &args.module_allowlist {
-        allowlist.modules = AllowList::load(path)?;
-    }
-
-    if let Some(path) = &args.source_allowlist {
-        allowlist.source_files = AllowList::load(path)?;
-    }
+    let source_allowlist = args
+        .source_allowlist
+        .map(AllowList::load)
+        .unwrap_or_else(|| Ok(AllowList::default()))
+        .context("loading source allowlist")?;
 
     let mut coverage = BinaryCoverage::default();
     let loader = Arc::new(Loader::new());
-    let cache = Arc::new(DebugInfoCache::new(allowlist.source_files.clone()));
+    let cache = Arc::new(DebugInfoCache::new(source_allowlist.clone()));
 
     let t = std::time::Instant::now();
     precache_target(&args.command[0], &loader, &cache)?;
@@ -84,7 +86,7 @@ fn main() -> Result<()> {
 
             let t = std::time::Instant::now();
             let recorded = CoverageRecorder::new(cmd)
-                .allowlist(allowlist.clone())
+                .module_allowlist(module_allowlist.clone())
                 .loader(loader.clone())
                 .debuginfo_cache(cache.clone())
                 .timeout(timeout)
@@ -102,7 +104,7 @@ fn main() -> Result<()> {
 
         let t = std::time::Instant::now();
         let recorded = CoverageRecorder::new(cmd)
-            .allowlist(allowlist.clone())
+            .module_allowlist(module_allowlist)
             .loader(loader)
             .debuginfo_cache(cache)
             .timeout(timeout)
@@ -118,8 +120,8 @@ fn main() -> Result<()> {
 
     match args.output {
         OutputFormat::ModOff => dump_modoff(&coverage)?,
-        OutputFormat::Source => dump_source_line(&coverage, allowlist.source_files)?,
-        OutputFormat::Cobertura => dump_cobertura(&coverage, allowlist.source_files)?,
+        OutputFormat::Source => dump_source_line(&coverage, source_allowlist)?,
+        OutputFormat::Cobertura => dump_cobertura(&coverage, source_allowlist)?,
     }
 
     Ok(())

src/agent/coverage/src/allowlist.rs

@@ -5,31 +5,6 @@ use anyhow::Result;
 use regex::{Regex, RegexSet};
 use std::path::Path;
 
-#[derive(Clone, Debug, Default)]
-pub struct TargetAllowList {
-    pub modules: AllowList,
-    pub source_files: AllowList,
-}
-
-impl TargetAllowList {
-    pub fn new(modules: AllowList, source_files: AllowList) -> Self {
-        Self {
-            modules,
-            source_files,
-        }
-    }
-
-    #[allow(clippy::field_reassign_with_default)]
-    pub fn extend(&self, other: &Self) -> Self {
-        let mut new = Self::default();
-
-        new.modules = self.modules.extend(&other.modules);
-        new.source_files = self.source_files.extend(&other.source_files);
-
-        new
-    }
-}
-
 #[derive(Clone, Debug)]
 pub struct AllowList {
     allow: RegexSet,

src/agent/coverage/src/binary.rs

@@ -11,7 +11,7 @@ pub use debuggable_module::{block, path::FilePath, Offset};
 use symbolic::debuginfo::Object;
 use symbolic::symcache::{SymCache, SymCacheConverter};
 
-use crate::allowlist::{AllowList, TargetAllowList};
+use crate::allowlist::AllowList;
 
 #[derive(Clone, Debug, Default, Eq, PartialEq)]
 pub struct BinaryCoverage {
@@ -214,7 +214,7 @@ impl CachedDebugInfo {
 
 pub fn find_coverage_sites(
     module: &dyn Module,
-    allowlist: &TargetAllowList,
+    source_allowlist: &AllowList,
 ) -> Result<ModuleBinaryCoverage> {
     let debuginfo = module.debuginfo()?;
 
@@ -232,7 +232,7 @@ pub fn find_coverage_sites(
     for function in debuginfo.functions() {
         if let Some(location) = symcache.lookup(function.offset.0).next() {
             if let Some(file) = location.file() {
-                if !allowlist.source_files.is_allowed(file.full_path()) {
+                if !source_allowlist.is_allowed(file.full_path()) {
                     debug!(
                         "skipping sweep of `{}:{}` due to excluded source path `{}`",
                         module.executable_path(),
@@ -253,7 +253,7 @@ pub fn find_coverage_sites(
 
                     // Apply allowlists per block, to account for inlining. The `location` values
                     // here describe the top of the inline-inclusive call stack.
-                    if !allowlist.source_files.is_allowed(path) {
+                    if !source_allowlist.is_allowed(path) {
                         continue;
                     }
 

src/agent/coverage/src/lib.rs

@@ -12,7 +12,7 @@ pub mod source;
 mod timer;
 
 #[doc(inline)]
-pub use allowlist::{AllowList, TargetAllowList};
+pub use allowlist::AllowList;
 
 #[doc(inline)]
 pub use record::{CoverageRecorder, Recorded};

src/agent/coverage/src/record.rs

@@ -8,8 +8,8 @@ use std::time::Duration;
 use anyhow::Result;
 use debuggable_module::loader::Loader;
 
-use crate::allowlist::TargetAllowList;
 use crate::binary::{BinaryCoverage, DebugInfoCache};
+use crate::AllowList;
 
 #[cfg(target_os = "linux")]
 pub mod linux;
@@ -18,7 +18,7 @@ pub mod linux;
 pub mod windows;
 
 pub struct CoverageRecorder {
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
     cache: Arc<DebugInfoCache>,
     cmd: Command,
     loader: Arc<Loader>,
@@ -30,22 +30,20 @@ impl CoverageRecorder {
         cmd.stdout(Stdio::piped());
         cmd.stderr(Stdio::piped());
 
-        let allowlist = TargetAllowList::default();
-        let cache = Arc::new(DebugInfoCache::new(allowlist.source_files.clone()));
         let loader = Arc::new(Loader::new());
         let timeout = Duration::from_secs(5);
 
         Self {
-            allowlist,
-            cache,
+            module_allowlist: AllowList::default(),
+            cache: Arc::new(DebugInfoCache::new(AllowList::default())),
             cmd,
             loader,
             timeout,
         }
     }
 
-    pub fn allowlist(mut self, allowlist: TargetAllowList) -> Self {
-        self.allowlist = allowlist;
+    pub fn module_allowlist(mut self, module_allowlist: AllowList) -> Self {
+        self.module_allowlist = module_allowlist;
         self
     }
 
@@ -82,7 +80,7 @@ impl CoverageRecorder {
             let child_pid = child_pid.clone();
 
             timer::timed(self.timeout, move || {
-                let mut recorder = LinuxRecorder::new(&loader, self.allowlist, &self.cache);
+                let mut recorder = LinuxRecorder::new(&loader, self.module_allowlist, &self.cache);
                 let mut dbg = Debugger::new(&mut recorder);
                 let child = dbg.spawn(self.cmd)?;
 
@@ -128,7 +126,8 @@ impl CoverageRecorder {
         let loader = self.loader.clone();
 
         crate::timer::timed(self.timeout, move || {
-            let mut recorder = WindowsRecorder::new(&loader, self.allowlist, &self.cache);
+            let mut recorder =
+                WindowsRecorder::new(&loader, self.module_allowlist, self.cache.as_ref());
             let (mut dbg, child) = Debugger::init(self.cmd, &mut recorder)?;
             dbg.run(&mut recorder)?;
 

src/agent/coverage/src/record/linux.rs

@@ -14,11 +14,11 @@ use pete::Tracee;
 pub mod debugger;
 use debugger::{DebugEventHandler, DebuggerContext, ModuleImage};
 
-use crate::allowlist::TargetAllowList;
+use crate::allowlist::AllowList;
 use crate::binary::{BinaryCoverage, DebugInfoCache};
 
 pub struct LinuxRecorder<'cache, 'data> {
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
     cache: &'cache DebugInfoCache,
     pub coverage: BinaryCoverage,
     loader: &'data Loader,
@@ -28,14 +28,14 @@ pub struct LinuxRecorder<'cache, 'data> {
 impl<'cache, 'data> LinuxRecorder<'cache, 'data> {
     pub fn new(
         loader: &'data Loader,
-        allowlist: TargetAllowList,
+        module_allowlist: AllowList,
         cache: &'cache DebugInfoCache,
     ) -> Self {
         let coverage = BinaryCoverage::default();
         let modules = BTreeMap::new();
 
         Self {
-            allowlist,
+            module_allowlist,
             cache,
             coverage,
             loader,
@@ -80,7 +80,7 @@ impl<'cache, 'data> LinuxRecorder<'cache, 'data> {
 
         let path = image.path();
 
-        if !self.allowlist.modules.is_allowed(path) {
+        if !self.module_allowlist.is_allowed(path) {
             debug!("not inserting denylisted module: {path}");
             return Ok(());
         }

src/agent/coverage/src/record/windows.rs

@@ -13,8 +13,8 @@ use debuggable_module::windows::WindowsModule;
 use debuggable_module::{Module, Offset};
 use debugger::{BreakpointId, BreakpointType, DebugEventHandler, Debugger, ModuleLoadInfo};
 
-use crate::allowlist::TargetAllowList;
 use crate::binary::{BinaryCoverage, DebugInfoCache};
+use crate::AllowList;
 
 // For a new module image, we defer setting coverage breakpoints until exit from one of these
 // functions (when present). This avoids breaking hotpatching routines in the ASan interceptor
@@ -26,7 +26,7 @@ const PROCESS_IMAGE_DEFERRAL_TRIGGER: &str = "__asan::AsanInitInternal(";
 const LIBRARY_IMAGE_DEFERRAL_TRIGGER: &str = "DllMain(";
 
 pub struct WindowsRecorder<'cache, 'data> {
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
     breakpoints: Breakpoints,
     cache: &'cache DebugInfoCache,
     deferred_breakpoints: BTreeMap<BreakpointId, (Breakpoint, DeferralState)>,
@@ -39,7 +39,7 @@ pub struct WindowsRecorder<'cache, 'data> {
 impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
     pub fn new(
         loader: &'data Loader,
-        allowlist: TargetAllowList,
+        module_allowlist: AllowList,
         cache: &'cache DebugInfoCache,
     ) -> Self {
         let breakpoints = Breakpoints::default();
@@ -49,7 +49,7 @@ impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
         let stop_error = None;
 
         Self {
-            allowlist,
+            module_allowlist,
             breakpoints,
             cache,
             deferred_breakpoints,
@@ -60,12 +60,12 @@ impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
         }
     }
 
-    pub fn allowlist(&self) -> &TargetAllowList {
-        &self.allowlist
+    pub fn module_allowlist(&self) -> &AllowList {
+        &self.module_allowlist
     }
 
-    pub fn allowlist_mut(&mut self) -> &mut TargetAllowList {
-        &mut self.allowlist
+    pub fn module_allowlist_mut(&mut self) -> &mut AllowList {
+        &mut self.module_allowlist
     }
 
     fn try_on_create_process(&mut self, dbg: &mut Debugger, module: &ModuleLoadInfo) -> Result<()> {
@@ -163,7 +163,7 @@ impl<'cache, 'data> WindowsRecorder<'cache, 'data> {
             return Ok(());
         }
 
-        if !self.allowlist.modules.is_allowed(&path) {
+        if !self.module_allowlist.is_allowed(&path) {
             debug!("not inserting denylisted module: {path}");
             return Ok(());
         }

src/agent/coverage/src/source.rs

@@ -51,14 +51,14 @@ impl From<Line> for u32 {
 
 pub fn binary_to_source_coverage(
     binary: &BinaryCoverage,
-    allowlist: impl Into<Option<AllowList>>,
+    source_allowlist: impl Into<Option<AllowList>>,
 ) -> Result<SourceCoverage> {
     use std::collections::btree_map::Entry;
 
     use symbolic::debuginfo::Object;
     use symbolic::symcache::{SymCache, SymCacheConverter};
 
-    let allowlist = allowlist.into().unwrap_or_default();
+    let source_allowlist = source_allowlist.into().unwrap_or_default();
     let loader = Loader::new();
 
     let mut source = SourceCoverage::default();
@@ -106,7 +106,7 @@ pub fn binary_to_source_coverage(
 
                         if let Some(file) = location.file() {
                             // Only include relevant inlinees.
-                            if !allowlist.is_allowed(&file.full_path()) {
+                            if !source_allowlist.is_allowed(&file.full_path()) {
                                 continue;
                             }
 

src/agent/coverage/tests/snapshot.rs

@@ -0,0 +1,81 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+#[test]
+#[cfg(all(target_os = "windows", feature = "slow-tests"))]
+fn windows_snapshot_tests() {
+    use coverage::{binary::DebugInfoCache, source::Line, AllowList};
+    use debuggable_module::path::FilePath;
+    use std::fmt::Write;
+    use std::time::Duration;
+
+    insta::glob!("windows", "*.cpp", |path| {
+        let file_name = path.file_name().unwrap();
+
+        // locate appropriate compiler
+        let mut cl_exe =
+            cc::windows_registry::find("x86_64-pc-windows-msvc", "cl.exe").expect("finding cl.exe");
+
+        // path will have \\?\ prefix, cl.exe doesn't like it
+        let input_path = dunce::canonicalize(path).unwrap();
+
+        // directory to compile into
+        let build_in = tempfile::tempdir().expect("creating tempdir");
+
+        let output = cl_exe
+            .arg("/EHsc")
+            .arg("/Zi")
+            .arg("/O2")
+            .arg(&input_path)
+            .current_dir(build_in.path())
+            .spawn()
+            .expect("launching compiler")
+            .wait_with_output()
+            .expect("waiting for compiler to finish");
+
+        assert!(output.status.success(), "cl.exe failed: {:?}", output);
+
+        let exe_name = {
+            let mut cwd = build_in.path().to_path_buf();
+            cwd.push(file_name);
+            cwd.set_extension("exe");
+            cwd
+        };
+
+        // filter to just the input test file:
+        let source_allowlist = AllowList::parse(&input_path.to_string_lossy()).unwrap();
+
+        let exe_cmd = std::process::Command::new(&exe_name);
+        let recorded = coverage::CoverageRecorder::new(exe_cmd)
+            .timeout(Duration::from_secs(120))
+            .debuginfo_cache(DebugInfoCache::new(source_allowlist.clone()))
+            .record()
+            .unwrap();
+
+        // generate source-line coverage info
+        let source =
+            coverage::source::binary_to_source_coverage(&recorded.coverage, source_allowlist)
+                .expect("binary_to_source_coverage");
+
+        let file_coverage = source
+            .files
+            .get(&FilePath::new(input_path.to_string_lossy()).unwrap())
+            .expect("coverage for input");
+
+        let mut result = String::new();
+
+        let file_source = std::fs::read_to_string(input_path).expect("reading source file");
+        for (ix, content) in file_source.lines().enumerate() {
+            let line = Line::new((ix + 1) as u32).unwrap();
+            let prefix = if file_coverage.lines.contains_key(&line) {
+                "[✔]"
+            } else {
+                "[ ]"
+            };
+
+            writeln!(result, "{prefix} {content}").unwrap();
+        }
+
+        insta::assert_snapshot!(result);
+    });
+}

src/agent/coverage/tests/snapshots/snapshot__windows_snapshot_tests.snap

@@ -0,0 +1,20 @@
+---
+source: coverage/tests/snapshot.rs
+expression: result
+input_file: coverage/tests/windows/inlinee.cpp
+---
+[ ] #include <iostream>
+[ ] 
+[ ] __declspec(dllexport) void test();
+[ ] 
+[ ] int main()
+[✔] {
+[✔]     std::cout << "Before\n";
+[✔]     test();
+[✔]     std::cout << "After\n";
+[✔] }
+[ ] 
+[ ] __declspec(dllexport) void test() {
+[✔]     std::cout << "Hello World!\n";
+[ ] }
+

src/agent/coverage/tests/windows/inlinee.cpp

@@ -0,0 +1,14 @@
+#include <iostream>
+
+__declspec(dllexport) void test();
+
+int main()
+{
+    std::cout << "Before\n";
+    test();
+    std::cout << "After\n";
+}
+
+__declspec(dllexport) void test() {
+    std::cout << "Hello World!\n";
+}

src/agent/onefuzz-task/src/tasks/coverage/generic.rs

@@ -11,7 +11,7 @@ use std::time::Duration;
 use anyhow::{bail, Context, Result};
 use async_trait::async_trait;
 use cobertura::CoberturaCoverage;
-use coverage::allowlist::{AllowList, TargetAllowList};
+use coverage::allowlist::AllowList;
 use coverage::binary::{BinaryCoverage, DebugInfoCache};
 use coverage::record::CoverageRecorder;
 use coverage::source::{binary_to_source_coverage, SourceCoverage};
@@ -206,10 +206,17 @@ impl CoverageTask {
     }
 }
 
+#[derive(Clone, Debug, Default)]
+struct TargetAllowList {
+    modules: AllowList,
+    source_files: AllowList,
+}
+
 struct TaskContext<'a> {
     config: &'a Config,
     coverage: BinaryCoverage,
-    allowlist: TargetAllowList,
+    module_allowlist: AllowList,
+    source_allowlist: AllowList,
     heartbeat: Option<TaskHeartbeatClient>,
     cache: Arc<DebugInfoCache>,
 }
@@ -236,7 +243,8 @@ impl<'a> TaskContext<'a> {
         Ok(Self {
             config,
             coverage,
-            allowlist,
+            module_allowlist: allowlist.modules,
+            source_allowlist: allowlist.source_files,
             heartbeat,
             cache: Arc::new(cache),
         })
@@ -285,14 +293,14 @@ impl<'a> TaskContext<'a> {
     }
 
     async fn record_impl(&mut self, input: &Path) -> Result<BinaryCoverage> {
-        let allowlist = self.allowlist.clone();
+        let module_allowlist = self.module_allowlist.clone();
         let cmd = self.command_for_input(input).await?;
         let timeout = self.config.timeout();
         let cache = self.cache.clone();
         let recorded = spawn_blocking(move || {
             CoverageRecorder::new(cmd)
                 .debuginfo_cache(cache)
-                .allowlist(allowlist)
+                .module_allowlist(module_allowlist)
                 .timeout(timeout)
                 .record()
         })
@@ -487,11 +495,11 @@ impl<'a> TaskContext<'a> {
 
     async fn source_coverage(&self) -> Result<SourceCoverage> {
         // Must be owned due to `spawn_blocking()` lifetimes.
-        let allowlist = self.allowlist.clone();
+        let allowlist = self.source_allowlist.clone();
         let binary = self.coverage.clone();
 
         // Conversion to source coverage heavy on blocking I/O.
-        spawn_blocking(move || binary_to_source_coverage(&binary, allowlist.source_files)).await?
+        spawn_blocking(move || binary_to_source_coverage(&binary, allowlist)).await?
     }
 }
 

src/ci/agent.sh

@@ -37,7 +37,7 @@ export RUST_BACKTRACE=full
 
 # Run tests and collect coverage 
 # https://github.com/taiki-e/cargo-llvm-cov
-cargo llvm-cov nextest --all-targets --locked --workspace --lcov --output-path "$output_dir/lcov.info"
+cargo llvm-cov nextest --all-targets --features slow-tests --locked --workspace --lcov --output-path "$output_dir/lcov.info"
 
 # TODO: re-enable integration tests.
 # cargo test --release --manifest-path ./onefuzz-task/Cargo.toml --features integration_test -- --nocapture

src/proxy-manager/Cargo.lock

@@ -1064,9 +1064,9 @@ dependencies = [
 
 [[package]]
 name = "quick-xml"
-version = "0.29.0"
+version = "0.30.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "81b9228215d82c7b61490fec1de287136b5de6f5700f6e58ea9ad61a7964ca51"
+checksum = "eff6510e86862b57b210fd8cbe8ed3f0d7d600b9c2863cd4549a2e033c66e956"
 dependencies = [
  "memchr",
  "serde",