Skip to content

Commit f1cf7f9

Browse files
authored
refactor: make cargo wdk integration tests faster (#553)
1 parent 6d1cb13 commit f1cf7f9

9 files changed

Lines changed: 300 additions & 64 deletions

File tree

.gitignore

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1 @@
11
target/
2-
3-
# ignore lock file created during cargo-wdk's integration test runs
4-
crates/cargo-wdk/cargo-wdk-test.lock

Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/cargo-wdk/Cargo.toml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ cargo_metadata.workspace = true
1616
clap = { workspace = true, features = ["derive"] }
1717
clap-cargo.workspace = true
1818
clap-verbosity-flag.workspace = true
19-
fs4.workspace = true
2019
include_dir.workspace = true
2120
mockall.workspace = true
2221
mockall_double.workspace = true
@@ -25,6 +24,11 @@ thiserror.workspace = true
2524
tracing.workspace = true
2625
tracing-subscriber = { workspace = true, features = ["env-filter"] }
2726
wdk-build.workspace = true
27+
windows = { workspace = true, features = [
28+
"Win32_Foundation",
29+
"Win32_System_Threading",
30+
"Win32_Security",
31+
] }
2832

2933
[dev-dependencies]
3034
assert_cmd.workspace = true

crates/cargo-wdk/src/actions/build/error.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ pub enum PackageTaskError {
7070
VerifyCertExistsInStoreInvalidCommandOutput(#[source] FromUtf8Error),
7171
#[error("Error generating certificate to cert store using makecert")]
7272
CertGenerationInStoreCommand(#[source] CommandError),
73+
#[error("Error while acquiring mutex for generating certificate. HRESULT: {0:#x}")]
74+
CertMutexError(i32),
7375
#[error("Error signing driver binary using signtool")]
7476
DriverBinarySignCommand(#[source] CommandError),
7577
#[error("Error verifying signed driver binary using signtool")]

crates/cargo-wdk/src/actions/build/package_task.rs

Lines changed: 173 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
//! validating, verifying and generating artefacts for the driver package.
99
1010
use std::{
11+
ffi::{CStr, CString},
12+
marker::PhantomData,
1113
ops::RangeFrom,
1214
path::{Path, PathBuf},
1315
result::Result,
@@ -16,6 +18,13 @@ use std::{
1618
use mockall_double::double;
1719
use tracing::{debug, info, warn};
1820
use wdk_build::{CpuArchitecture, DriverConfig};
21+
use windows::{
22+
Win32::{
23+
Foundation::{CloseHandle, GetLastError, HANDLE, WAIT_ABANDONED, WAIT_OBJECT_0},
24+
System::Threading::{CreateMutexA, INFINITE, ReleaseMutex, WaitForSingleObject},
25+
},
26+
core::{Error as WinError, PCSTR},
27+
};
1928

2029
#[double]
2130
use crate::providers::{exec::CommandExec, fs::Fs, wdk_build::WdkBuild};
@@ -368,8 +377,26 @@ impl<'a> PackageTask<'a> {
368377
if self.is_self_signed_certificate_in_store()? {
369378
self.create_cert_file_from_store()?;
370379
} else {
371-
self.create_self_signed_cert_in_store()?;
380+
// This mutex prevents multiple instances of this app from racing to
381+
// create a cert in the store. It is not a correctness problem. We
382+
// just don't want to litter the store with certs especially during
383+
// tests when there are lots of parallel runs
384+
let mutex_name = CString::new("WDRCertStoreMutex_bd345cf9330") // Unique enough
385+
.expect("string is a valid C string");
386+
let mutex = NamedMutex::acquire(&mutex_name)
387+
.map_err(|e| PackageTaskError::CertMutexError(e.code().0))?;
388+
debug!("Acquired cert store mutex");
389+
390+
// Check again for an existing cert. Another instance might have
391+
// created it while we waited for the mutex
392+
if self.is_self_signed_certificate_in_store()? {
393+
drop(mutex);
394+
self.create_cert_file_from_store()?;
395+
} else {
396+
self.create_self_signed_cert_in_store()?;
397+
}
372398
}
399+
373400
Ok(())
374401
}
375402

@@ -527,6 +554,62 @@ impl<'a> PackageTask<'a> {
527554
Ok(())
528555
}
529556
}
557+
558+
/// An RAII wrapper over a Win API named mutex
559+
struct NamedMutex {
560+
handle: HANDLE,
561+
// `ReleaseMutex` requires that it is called
562+
// only by threads that own the mutex handle.
563+
// Being `!Send` ensures that's always the case.
564+
_not_send: PhantomData<*const ()>,
565+
}
566+
567+
impl NamedMutex {
568+
/// Acquires named mutex
569+
pub fn acquire(name: &CStr) -> Result<Self, WinError> {
570+
fn get_last_error() -> WinError {
571+
// SAFETY: We have to just assume this function is safe to call
572+
// because the windows crate has no documentation for it and
573+
// the MSDN documentation does not specify any preconditions
574+
// for calling it
575+
unsafe { GetLastError().into() }
576+
}
577+
578+
// SAFETY: The name ptr is valid because it comes from a CStr
579+
let handle = unsafe { CreateMutexA(None, false, PCSTR(name.as_ptr().cast()))? };
580+
if handle.is_invalid() {
581+
return Err(get_last_error());
582+
}
583+
584+
// SAFETY: The handle is valid since it was created right above
585+
match unsafe { WaitForSingleObject(handle, INFINITE) } {
586+
res if res == WAIT_OBJECT_0 || res == WAIT_ABANDONED => Ok(Self {
587+
handle,
588+
_not_send: PhantomData,
589+
}),
590+
_ => {
591+
// SAFETY: The handle is valid since it was created right above
592+
unsafe { CloseHandle(handle)? };
593+
Err(get_last_error())
594+
}
595+
}
596+
}
597+
}
598+
599+
impl Drop for NamedMutex {
600+
fn drop(&mut self) {
601+
// SAFETY: the handle is guaranteed to be valid
602+
// because this type itself created it and it
603+
// was never exposed outside. Also the requirement
604+
// that the calling thread must own the handle
605+
// is upheld because this type is `!Send`
606+
let _ = unsafe { ReleaseMutex(self.handle) };
607+
608+
// SAFETY: the handle is valid as explained above.
609+
let _ = unsafe { CloseHandle(self.handle) };
610+
}
611+
}
612+
530613
#[cfg(test)]
531614
mod tests {
532615
use std::{
@@ -715,4 +798,93 @@ mod tests {
715798
);
716799
}
717800
}
801+
802+
mod named_mutex {
803+
use std::{
804+
ffi::CString,
805+
sync::{
806+
Barrier,
807+
atomic::{AtomicUsize, Ordering},
808+
},
809+
thread,
810+
time::Duration,
811+
};
812+
813+
use super::super::NamedMutex;
814+
815+
/// Tests that two threads successfully acquire `NamedMutex`
816+
/// and it prevents them from running concurrently.
817+
#[test]
818+
fn acquire_works_correctly() {
819+
// The way this test work is:
820+
// 1. We create two threads that start at the same time thanks
821+
// to a barrier
822+
// 2. Both increment a counter `active` while they run holding
823+
// the mutex
824+
// 3. Both also increment another counter `completed` when they finish
825+
// 4. We verify that `active` never exceeds 1 i.e. there's no concurrent
826+
// execution and `completed` is 2 at the end i.e. both threads run to completion
827+
828+
let barrier = Barrier::new(2);
829+
let active = AtomicUsize::new(0);
830+
let completed = AtomicUsize::new(0);
831+
832+
thread::scope(|s| {
833+
for _ in 0..2 {
834+
s.spawn(|| {
835+
let name =
836+
CString::new("happy_path_d44f8b8a817").expect("it is a valid C string");
837+
838+
barrier.wait();
839+
let guard = NamedMutex::acquire(name.as_c_str())
840+
.expect("thread should acquire mutex");
841+
842+
let active_prev = active.fetch_add(1, Ordering::SeqCst);
843+
assert_eq!(active_prev, 0, "named mutex allowed concurrent access");
844+
845+
thread::sleep(Duration::from_millis(100));
846+
847+
let active_prev = active.fetch_sub(1, Ordering::SeqCst);
848+
assert_eq!(active_prev, 1, "active counter should drop back to zero");
849+
850+
drop(guard);
851+
852+
completed.fetch_add(1, Ordering::SeqCst);
853+
});
854+
}
855+
});
856+
857+
assert_eq!(completed.load(Ordering::SeqCst), 2);
858+
assert_eq!(active.load(Ordering::SeqCst), 0);
859+
}
860+
861+
/// Tests that `NamedMutex` can be acquired even after the previous
862+
/// owner abandoned it (e.g. crashed) without releasing
863+
///
864+
/// What we are really testing here is `WaitForSingleObject`
865+
/// inside `NamedMutex::acquire` returning `WAIT_ABANDONED`
866+
#[test]
867+
fn acquire_works_when_abandoned() {
868+
fn acquire_mutex() -> NamedMutex {
869+
let name =
870+
CString::new("abandoned_owner_d44f8b8a817").expect("it is a valid C string");
871+
NamedMutex::acquire(name.as_c_str()).expect("thread should acquire mutex")
872+
}
873+
874+
// Acquire the mutex on a thread and abandon it
875+
thread::scope(|s| {
876+
s.spawn(|| {
877+
let guard = acquire_mutex();
878+
// Simulate an abnormal exit while still holding the mutex to trigger the
879+
// WAIT_ABANDONED path for the next owner.
880+
std::mem::forget(guard);
881+
});
882+
});
883+
884+
// Try to acquire the same mutex from the main thread
885+
// which should succeed despite the abandonment above
886+
let guard = acquire_mutex();
887+
drop(guard);
888+
}
889+
}
718890
}

crates/cargo-wdk/src/actions/build/tests.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2159,7 +2159,6 @@ impl TestBuildAction {
21592159
command == expected_certmgr_command && args == expected_certmgr_args
21602160
},
21612161
)
2162-
.once()
21632162
.returning(move |_, _, _, _| match override_output.clone() {
21642163
Some(output) => match output.status.code() {
21652164
Some(0) => Ok(Output {

crates/cargo-wdk/tests/build_command_test.rs

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@ use std::{
88

99
use assert_cmd::prelude::*;
1010
use sha2::{Digest, Sha256};
11-
use test_utils::{set_crt_static_flag, with_env, with_file_lock};
11+
use test_utils::{set_crt_static_flag, with_env, with_mutex};
1212

1313
const STAMPINF_VERSION_ENV_VAR: &str = "STAMPINF_VERSION";
1414

1515
#[test]
1616
fn mixed_package_kmdf_workspace_builds_successfully() {
17-
let stdout = with_file_lock(|| {
17+
let stdout = with_mutex("mixed_package_kmdf_workspace", || {
1818
run_cargo_clean("tests/mixed-package-kmdf-workspace");
1919
run_build_cmd("tests/mixed-package-kmdf-workspace")
2020
});
@@ -56,17 +56,17 @@ fn kmdf_driver_builds_successfully() {
5656
assert!(output.status.success());
5757
}
5858

59-
with_file_lock(|| clean_and_build_driver_project("kmdf", None));
59+
clean_and_build_driver_project("kmdf", None);
6060
}
6161

6262
#[test]
6363
fn umdf_driver_builds_successfully() {
64-
with_file_lock(|| clean_and_build_driver_project("umdf", None));
64+
clean_and_build_driver_project("umdf", None);
6565
}
6666

6767
#[test]
6868
fn wdm_driver_builds_successfully() {
69-
with_file_lock(|| clean_and_build_driver_project("wdm", None));
69+
clean_and_build_driver_project("wdm", None);
7070
}
7171

7272
#[test]
@@ -80,7 +80,7 @@ fn wdm_driver_builds_successfully_with_given_version() {
8080
fn emulated_workspace_builds_successfully() {
8181
let emulated_workspace_path = "tests/emulated-workspace";
8282
let umdf_driver_workspace_path = format!("{emulated_workspace_path}/umdf-driver-workspace");
83-
let stdout = with_file_lock(|| {
83+
let stdout = with_mutex("emulated_workspace", || {
8484
run_cargo_clean(&umdf_driver_workspace_path);
8585
run_build_cmd(emulated_workspace_path)
8686
});
@@ -97,23 +97,25 @@ fn clean_and_build_driver_project(driver_type: &str, driver_version: Option<&str
9797
let driver_name = format!("{driver_type}-driver");
9898
let driver_path = format!("tests/{driver_name}");
9999

100-
run_cargo_clean(&driver_path);
101-
let stdout = run_build_cmd(&driver_path);
100+
with_mutex(&driver_path, || {
101+
run_cargo_clean(&driver_path);
102+
let stdout = run_build_cmd(&driver_path);
102103

103-
assert!(stdout.contains(&format!("Building package {driver_name}")));
104+
assert!(stdout.contains(&format!("Building package {driver_name}")));
104105

105-
let driver_binary_extension = match driver_type {
106-
"kmdf" | "wdm" => "sys",
107-
"umdf" => "dll",
108-
_ => panic!("Unsupported driver type: {driver_type}"),
109-
};
106+
let driver_binary_extension = match driver_type {
107+
"kmdf" | "wdm" => "sys",
108+
"umdf" => "dll",
109+
_ => panic!("Unsupported driver type: {driver_type}"),
110+
};
110111

111-
verify_driver_package_files(
112-
&driver_path,
113-
&driver_name,
114-
driver_binary_extension,
115-
driver_version,
116-
);
112+
verify_driver_package_files(
113+
&driver_path,
114+
&driver_name,
115+
driver_binary_extension,
116+
driver_version,
117+
);
118+
});
117119
}
118120

119121
fn run_cargo_clean(driver_path: &str) {

0 commit comments

Comments
 (0)