Skip to content

Commit e300871

Browse files
committed
fix(verifier): download each image once and install it off the async runtime
Concurrent requests for an uncached image each downloaded it, and every finisher after the first ran remove_dir_all on the installed image while the others could be measuring from it, then raced the rename. Take a per-image lock before the cache check and hold it until the image is installed. Extraction, the manifest check and the install run in one spawn_blocking task that owns the lock guard and the temporary directory, so a caller that hits download_timeout cannot leave a still-running install to collide with the next download.
1 parent 2275694 commit e300871

1 file changed

Lines changed: 118 additions & 9 deletions

File tree

‎dstack/verifier/src/verification.rs‎

Lines changed: 118 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@
33
// SPDX-License-Identifier: Apache-2.0
44

55
use std::{
6+
collections::HashMap,
67
ffi::OsStr,
78
path::{Component, Path, PathBuf},
8-
sync::Arc,
9+
sync::{Arc, Mutex, PoisonError, Weak},
910
time::Duration,
1011
};
1112

@@ -26,7 +27,7 @@ use ra_tls::attestation::{
2627
};
2728
use serde::{Deserialize, Serialize};
2829
use sha2::{Digest as _, Sha256};
29-
use tokio::io::AsyncWriteExt;
30+
use tokio::{io::AsyncWriteExt, sync::Mutex as AsyncMutex};
3031
use tpm_qvl::verify::VerifiedReport as TpmVerifiedReport;
3132
use tracing::{debug, info, warn};
3233

@@ -121,6 +122,7 @@ pub struct CvmVerifier {
121122
pub download_url: String,
122123
pub download_timeout: Duration,
123124
pub attestation_verifier: Arc<AttestationVerifier>,
125+
download_locks: Mutex<HashMap<String, Weak<AsyncMutex<()>>>>,
124126
}
125127

126128
impl CvmVerifier {
@@ -135,9 +137,26 @@ impl CvmVerifier {
135137
download_url,
136138
download_timeout,
137139
attestation_verifier,
140+
download_locks: Mutex::default(),
138141
}
139142
}
140143

144+
/// One lock per image, held from the cache check until the image is
145+
/// installed, so concurrent requests for an image download it once.
146+
fn download_lock(&self, hex_os_image_hash: &str) -> Arc<AsyncMutex<()>> {
147+
let mut locks = self
148+
.download_locks
149+
.lock()
150+
.unwrap_or_else(PoisonError::into_inner);
151+
locks.retain(|_, lock| lock.strong_count() > 0);
152+
if let Some(lock) = locks.get(hex_os_image_hash).and_then(Weak::upgrade) {
153+
return lock;
154+
}
155+
let lock = Arc::default();
156+
locks.insert(hex_os_image_hash.to_owned(), Arc::downgrade(&lock));
157+
lock
158+
}
159+
141160
fn measurement_cache_dir(&self) -> PathBuf {
142161
Path::new(&self.image_cache_dir).join("measurements")
143162
}
@@ -1022,6 +1041,10 @@ impl CvmVerifier {
10221041
}
10231042

10241043
pub async fn download_image(&self, hex_os_image_hash: &str, dst_dir: &Path) -> Result<()> {
1044+
let guard = self.download_lock(hex_os_image_hash).lock_owned().await;
1045+
if dst_dir.join("metadata.json").exists() {
1046+
return Ok(());
1047+
}
10251048
let url = self
10261049
.download_url
10271050
.replace("{OS_IMAGE_HASH}", hex_os_image_hash);
@@ -1067,26 +1090,38 @@ impl CvmVerifier {
10671090
.context("Failed to write chunk to file")?;
10681091
}
10691092

1070-
let extracted_dir = tmp_dir.join("extracted");
1071-
fs_err::create_dir_all(&extracted_dir).context("Failed to create extraction directory")?;
1072-
10731093
file.flush()
10741094
.await
10751095
.context("Failed to flush image archive")?;
10761096
drop(file);
1077-
Self::extract_image_archive(&tarball_path, &extracted_dir)?;
10781097

1098+
let hex_os_image_hash = hex_os_image_hash.to_owned();
1099+
let dst_dir = dst_dir.to_owned();
1100+
tokio::task::spawn_blocking(move || {
1101+
// A blocking task outlives its caller: if the caller is cancelled
1102+
// (e.g. by `download_timeout`), this still runs to the end. Keep
1103+
// the lock until the install finishes, or the next download for
1104+
// this image would race it on `dst_dir`.
1105+
let _guard = guard;
1106+
Self::install_image(auto_delete_temp_dir.path(), &hex_os_image_hash, &dst_dir)
1107+
})
1108+
.await
1109+
.context("image install task failed")?
1110+
}
1111+
1112+
fn install_image(tmp_dir: &Path, hex_os_image_hash: &str, dst_dir: &Path) -> Result<()> {
1113+
let extracted_dir = tmp_dir.join("extracted");
1114+
fs_err::create_dir_all(&extracted_dir).context("Failed to create extraction directory")?;
1115+
Self::extract_image_archive(&tmp_dir.join("image.tar.gz"), &extracted_dir)?;
10791116
Self::verify_extracted_image(&extracted_dir, hex_os_image_hash)?;
10801117

10811118
if dst_dir.exists() {
10821119
fs_err::remove_dir_all(dst_dir).context("Failed to remove destination directory")?;
10831120
}
10841121
let dst_dir_parent = dst_dir.parent().context("Failed to get parent directory")?;
10851122
fs_err::create_dir_all(dst_dir_parent).context("Failed to create parent directory")?;
1086-
// Move the extracted files to the destination directory
10871123
fs_err::rename(extracted_dir, dst_dir)
1088-
.context("Failed to move extracted files to destination directory")?;
1089-
Ok(())
1124+
.context("Failed to move extracted files to destination directory")
10901125
}
10911126
}
10921127

@@ -1853,6 +1888,80 @@ mod tests {
18531888
}
18541889
}
18551890

1891+
#[tokio::test(flavor = "multi_thread")]
1892+
async fn concurrent_requests_download_an_image_once() {
1893+
let metadata = br#"{"cmdline":"","kernel":"bzImage","initrd":"initrd","bios":"ovmf.fd"}"#;
1894+
let files: &[(&str, &[u8])] = &[
1895+
("metadata.json", metadata),
1896+
("bzImage", b"kernel"),
1897+
("initrd", b"initrd"),
1898+
("ovmf.fd", b"firmware"),
1899+
];
1900+
let files_doc: String = files
1901+
.iter()
1902+
.map(|(name, payload)| format!("{} {name}\n", hex::encode(Sha256::digest(payload))))
1903+
.collect();
1904+
let hash = hex::encode(Sha256::digest(&files_doc));
1905+
let mut archive = tar::Builder::new(flate2::write::GzEncoder::new(
1906+
Vec::new(),
1907+
flate2::Compression::default(),
1908+
));
1909+
for (name, payload) in files
1910+
.iter()
1911+
.chain([&("sha256sum.txt", files_doc.as_bytes())])
1912+
{
1913+
let mut header = tar::Header::new_gnu();
1914+
header.set_size(payload.len() as u64);
1915+
header.set_mode(0o644);
1916+
archive.append_data(&mut header, name, *payload).unwrap();
1917+
}
1918+
let tarball = archive.into_inner().unwrap().finish().unwrap();
1919+
1920+
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
1921+
let url = format!(
1922+
"http://{}/{{OS_IMAGE_HASH}}",
1923+
listener.local_addr().unwrap()
1924+
);
1925+
let served = Arc::new(std::sync::atomic::AtomicUsize::new(0));
1926+
tokio::spawn({
1927+
let served = served.clone();
1928+
async move {
1929+
use tokio::io::AsyncReadExt;
1930+
while let Ok((mut stream, _)) = listener.accept().await {
1931+
served.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
1932+
let _ = stream.read(&mut [0; 4096]).await;
1933+
tokio::time::sleep(Duration::from_millis(100)).await;
1934+
let head = format!(
1935+
"HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n",
1936+
tarball.len()
1937+
);
1938+
let _ = stream.write_all(head.as_bytes()).await;
1939+
let _ = stream.write_all(&tarball).await;
1940+
}
1941+
}
1942+
});
1943+
1944+
let cache = tempfile::tempdir().unwrap();
1945+
let verifier = Arc::new(CvmVerifier::new(
1946+
cache.path().to_str().unwrap().into(),
1947+
url,
1948+
Duration::from_secs(10),
1949+
test_attestation_verifier(),
1950+
));
1951+
let dst_dir = verifier.images_dir().join(&hash);
1952+
let downloads: Vec<_> = (0..8)
1953+
.map(|_| {
1954+
let (verifier, hash, dst_dir) = (verifier.clone(), hash.clone(), dst_dir.clone());
1955+
tokio::spawn(async move { verifier.download_image(&hash, &dst_dir).await })
1956+
})
1957+
.collect();
1958+
for download in downloads {
1959+
download.await.unwrap().unwrap();
1960+
}
1961+
assert_eq!(served.load(std::sync::atomic::Ordering::SeqCst), 1);
1962+
assert_eq!(fs_err::read(dst_dir.join("bzImage")).unwrap(), b"kernel");
1963+
}
1964+
18561965
#[test]
18571966
fn image_archive_rejects_links_and_accepts_regular_files() {
18581967
let directory = tempfile::tempdir().unwrap();

0 commit comments

Comments
 (0)