From 5fd12c24bb2e11ec93b555104ca4f119c4147786 Mon Sep 17 00:00:00 2001 From: Bhavesh M Date: Wed, 27 Mar 2024 17:07:10 +0000 Subject: [PATCH 1/8] Checkpoint Signed-off-by: Gabriel Keller --- src/vmm/src/snapshot/mod.rs | 108 ++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index 2a19a5d5298..28daa1441d4 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -80,6 +80,22 @@ impl SnapshotHdr { version, } } + + fn load(reader: &mut R) -> Result { + let hdr: SnapshotHdr = bincode::DefaultOptions::new() + .with_limit(VM_STATE_DESERIALIZE_LIMIT) + .with_fixint_encoding() + .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after + // reading the header, there will be trailing bytes. + .deserialize_from(reader) + .map_err(|err| SnapshotError::Serde(err.to_string()))?; + + Ok(hdr) + } + + fn store(&self, writer: &mut W) -> Result<(), SnapshotError> { + bincode::serialize_into(writer, self).map_err(|err| SnapshotError::Serde(err.to_string())) + } } /// Firecracker snapshot type @@ -91,6 +107,98 @@ pub struct Snapshot { version: Version, } +#[derive(Debug, Serialize, Deserialize)] +pub struct SnapshotNew { + // The snapshot version we can handle + version: Version, + data: Data, +} + +impl Deserialize<'a>> SnapshotNew { + /// Helper function to deserialize an object from a reader + pub fn deserialize(reader: &mut T) -> Result + where + T: Read, + O: DeserializeOwned + Debug, + { + // flags below are those used by default by bincode::deserialize_from, plus `with_limit`. + bincode::DefaultOptions::new() + .with_limit(VM_STATE_DESERIALIZE_LIMIT) + .with_fixint_encoding() + .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after + // reading the header, there will be trailing bytes. + .deserialize_from(reader) + .map_err(|err| SnapshotError::Serde(err.to_string())) + } + + pub fn load_unchecked(reader: &mut R) -> Result where Data: DeserializeOwned + Debug { + let hdr: SnapshotHdr = Self::deserialize(reader)?; + if hdr.magic != SNAPSHOT_MAGIC_ID { + return Err(SnapshotError::InvalidMagic(hdr.magic)); + } + + let data: Data = Self::deserialize(reader)?; + Ok(Self { + version: hdr.version, + data + }) + } + + pub fn load(reader: &mut R, snapshot_len: usize) -> Result where Data: DeserializeOwned + Debug { + let mut crc_reader = CRC64Reader::new(reader); + + // Fail-fast if the snapshot length is too small + let raw_snapshot_len = snapshot_len + .checked_sub(std::mem::size_of::()) + .ok_or(SnapshotError::InvalidSnapshotSize)?; + + // Read everything apart from the CRC. + let mut snapshot = vec![0u8; raw_snapshot_len]; + crc_reader + .read_exact(&mut snapshot) + .map_err(|ref err| SnapshotError::Io(err.raw_os_error().unwrap_or(libc::EINVAL)))?; + + // Since the reader updates the checksum as bytes ar being read from it, the order of these + // 2 statements is important, we first get the checksum computed on the read bytes + // then read the stored checksum. + let computed_checksum = crc_reader.checksum(); + let stored_checksum: u64 = Self::deserialize(&mut crc_reader)?; + if computed_checksum != stored_checksum { + return Err(SnapshotError::Crc64(computed_checksum)); + } + + let mut snapshot_slice: &[u8] = snapshot.as_mut_slice(); + SnapshotNew::load_unchecked::<_>(&mut snapshot_slice) + } +} + +impl SnapshotNew { + /// Helper function to serialize an object to a writer + pub fn serialize(writer: &mut T, data: &O) -> Result<(), SnapshotError> + where + T: Write, + O: Serialize + Debug, + { + bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) + } + + pub fn save(&self, writer: &mut W) -> Result { + // Write magic value and snapshot version + Self::serialize(&mut writer, &SnapshotHdr::new(self.version.clone()))?; + // Write data + Self::serialize(&mut writer, self.data) + } + + pub fn save_with_crc(&self, writer: &mut W) -> Result { + let mut crc_writer = CRC64Writer::new(writer); + self.save(&mut crc_writer)?; + + // Now write CRC value + let checksum = crc_writer.checksum(); + Self::serialize(&mut crc_writer, &checksum) + } +} + impl Snapshot { /// Creates a new instance which can only be used to save a new snapshot. pub fn new(version: Version) -> Snapshot { From 547e06b9177451372e8fa091a97b0d059aa0f309 Mon Sep 17 00:00:00 2001 From: Bhavesh M Date: Sat, 20 Jul 2024 00:36:51 +0000 Subject: [PATCH 2/8] Checkpoint 2 Signed-off-by: Gabriel Keller --- src/vmm/src/persist.rs | 24 +- src/vmm/src/snapshot/mod.rs | 483 +++++++++++++----------------------- 2 files changed, 191 insertions(+), 316 deletions(-) diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs index 6c8058899f2..5c957db589b 100644 --- a/src/vmm/src/persist.rs +++ b/src/vmm/src/persist.rs @@ -31,7 +31,7 @@ use crate::device_manager::persist::ACPIDeviceManagerState; use crate::device_manager::persist::{DevicePersistError, DeviceStates}; use crate::logger::{info, warn}; use crate::resources::VmResources; -use crate::snapshot::Snapshot; +use crate::snapshot::{Snapshot, SnapshotHdr}; use crate::vmm_config::boot_source::BootSourceConfig; use crate::vmm_config::instance_info::InstanceInfo; use crate::vmm_config::machine_config::{HugePageConfig, MachineConfigUpdate, VmConfigError}; @@ -191,9 +191,10 @@ fn snapshot_state_to_file( .open(snapshot_path) .map_err(|err| SnapshotBackingFile("open", err))?; - let snapshot = Snapshot::new(SNAPSHOT_VERSION); + let snapshot_hdr = SnapshotHdr::new(SNAPSHOT_VERSION); + let snapshot = Snapshot::new(snapshot_hdr, microvm_state); snapshot - .save(&mut snapshot_file, microvm_state) + .save(&mut snapshot_file) .map_err(SerializeMicrovmState)?; snapshot_file .flush() @@ -475,15 +476,14 @@ pub enum SnapshotStateFromFileError { fn snapshot_state_from_file( snapshot_path: &Path, ) -> Result { - let snapshot = Snapshot::new(SNAPSHOT_VERSION); let mut snapshot_reader = File::open(snapshot_path).map_err(SnapshotStateFromFileError::Open)?; let metadata = std::fs::metadata(snapshot_path).map_err(SnapshotStateFromFileError::Meta)?; let snapshot_len = u64_to_usize(metadata.len()); - let state: MicrovmState = snapshot - .load_with_version_check(&mut snapshot_reader, snapshot_len) + let state: Snapshot = Snapshot::load(&mut snapshot_reader, snapshot_len) .map_err(SnapshotStateFromFileError::Load)?; - Ok(state) + + Ok(state.data) } /// Error type for [`guest_memory_from_file`]. @@ -732,10 +732,14 @@ mod tests { }; let mut buf = vec![0; 10000]; - Snapshot::serialize(&mut buf.as_mut_slice(), µvm_state).unwrap(); - let restored_microvm_state: MicrovmState = - Snapshot::deserialize(&mut buf.as_slice()).unwrap(); + let snapshot_hdr = SnapshotHdr::new(Version::new(1, 0, 42)); + let snapshot = Snapshot::new(snapshot_hdr, microvm_state); + snapshot.save(&mut buf.as_mut_slice()).unwrap(); + + let restored_snapshot: Snapshot = + Snapshot::load(&mut buf.as_slice(), buf.len()).unwrap(); + let restored_microvm_state = restored_snapshot.data; assert_eq!(restored_microvm_state.vm_info, microvm_state.vm_info); assert_eq!( diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index 28daa1441d4..f743cc12197 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -66,7 +66,7 @@ pub enum SnapshotError { /// Firecracker snapshot header #[derive(Debug, Serialize, Deserialize)] -struct SnapshotHdr { +pub struct SnapshotHdr { /// magic value magic: u64, /// Snapshot data version @@ -74,14 +74,14 @@ struct SnapshotHdr { } impl SnapshotHdr { - fn new(version: Version) -> Self { + pub fn new(version: Version) -> Self { Self { magic: SNAPSHOT_MAGIC_ID, version, } } - fn load(reader: &mut R) -> Result { + pub fn load(reader: &mut R) -> Result { let hdr: SnapshotHdr = bincode::DefaultOptions::new() .with_limit(VM_STATE_DESERIALIZE_LIMIT) .with_fixint_encoding() @@ -89,32 +89,23 @@ impl SnapshotHdr { // reading the header, there will be trailing bytes. .deserialize_from(reader) .map_err(|err| SnapshotError::Serde(err.to_string()))?; - + Ok(hdr) } - fn store(&self, writer: &mut W) -> Result<(), SnapshotError> { + pub fn store(&self, writer: &mut W) -> Result<(), SnapshotError> { bincode::serialize_into(writer, self).map_err(|err| SnapshotError::Serde(err.to_string())) } } -/// Firecracker snapshot type -/// -/// A type used to store and load Firecracker snapshots of a particular version -#[derive(Debug)] -pub struct Snapshot { - // The snapshot version we can handle - version: Version, -} - #[derive(Debug, Serialize, Deserialize)] -pub struct SnapshotNew { +pub struct Snapshot { // The snapshot version we can handle - version: Version, - data: Data, + header: SnapshotHdr, + pub data: Data, } -impl Deserialize<'a>> SnapshotNew { +impl Deserialize<'a>> Snapshot { /// Helper function to deserialize an object from a reader pub fn deserialize(reader: &mut T) -> Result where @@ -131,7 +122,10 @@ impl Deserialize<'a>> SnapshotNew { .map_err(|err| SnapshotError::Serde(err.to_string())) } - pub fn load_unchecked(reader: &mut R) -> Result where Data: DeserializeOwned + Debug { + pub fn load_unchecked(reader: &mut R) -> Result + where + Data: DeserializeOwned + Debug, + { let hdr: SnapshotHdr = Self::deserialize(reader)?; if hdr.magic != SNAPSHOT_MAGIC_ID { return Err(SnapshotError::InvalidMagic(hdr.magic)); @@ -139,12 +133,15 @@ impl Deserialize<'a>> SnapshotNew { let data: Data = Self::deserialize(reader)?; Ok(Self { - version: hdr.version, - data + header: hdr, + data, }) } - pub fn load(reader: &mut R, snapshot_len: usize) -> Result where Data: DeserializeOwned + Debug { + pub fn load(reader: &mut R, snapshot_len: usize) -> Result + where + Data: DeserializeOwned + Debug, + { let mut crc_reader = CRC64Reader::new(reader); // Fail-fast if the snapshot length is too small @@ -168,25 +165,34 @@ impl Deserialize<'a>> SnapshotNew { } let mut snapshot_slice: &[u8] = snapshot.as_mut_slice(); - SnapshotNew::load_unchecked::<_>(&mut snapshot_slice) + Snapshot::load_unchecked::<_>(&mut snapshot_slice) } } -impl SnapshotNew { +impl Snapshot { /// Helper function to serialize an object to a writer - pub fn serialize(writer: &mut T, data: &O) -> Result<(), SnapshotError> + pub fn serialize(writer: &mut T, data: &O) -> Result where T: Write, O: Serialize + Debug, { - bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) + let mut buffer = Vec::new(); + bincode::serialize_into(&mut buffer, data) + .map_err(|err| SnapshotError::Serde(err.to_string()))?; + + writer + .write_all(&buffer) + .map_err(|err| SnapshotError::Serde(err.to_string()))?; + + Ok(buffer.len()) + // bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) } - pub fn save(&self, writer: &mut W) -> Result { + pub fn save(&self, mut writer: &mut W) -> Result { // Write magic value and snapshot version - Self::serialize(&mut writer, &SnapshotHdr::new(self.version.clone()))?; + Self::serialize(&mut writer, &SnapshotHdr::new(self.header.version.clone()))?; // Write data - Self::serialize(&mut writer, self.data) + Self::serialize(&mut writer, &self.data) } pub fn save_with_crc(&self, writer: &mut W) -> Result { @@ -199,141 +205,9 @@ impl SnapshotNew { } } -impl Snapshot { - /// Creates a new instance which can only be used to save a new snapshot. - pub fn new(version: Version) -> Snapshot { - Snapshot { version } - } - - /// Fetches snapshot data version. - pub fn get_format_version(reader: &mut T) -> Result - where - T: Read + Debug, - { - let hdr: SnapshotHdr = Self::deserialize(reader)?; - Ok(hdr.version) - } - - /// Helper function to deserialize an object from a reader - pub fn deserialize(reader: &mut T) -> Result - where - T: Read, - O: DeserializeOwned + Debug, - { - // flags below are those used by default by bincode::deserialize_from, plus `with_limit`. - bincode::DefaultOptions::new() - .with_limit(VM_STATE_DESERIALIZE_LIMIT) - .with_fixint_encoding() - .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after - // reading the header, there will be trailing bytes. - .deserialize_from(reader) - .map_err(|err| SnapshotError::Serde(err.to_string())) - } - - /// Helper function to serialize an object to a writer - pub fn serialize(writer: &mut T, data: &O) -> Result<(), SnapshotError> - where - T: Write, - O: Serialize + Debug, - { - bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) - } - - /// Attempts to load an existing snapshot without performing CRC or version validation. - /// - /// This will check that the snapshot magic value is correct. - fn unchecked_load(reader: &mut T) -> Result<(O, Version), SnapshotError> - where - T: Read + Debug, - O: DeserializeOwned + Debug, - { - let hdr: SnapshotHdr = Self::deserialize(reader)?; - if hdr.magic != SNAPSHOT_MAGIC_ID { - return Err(SnapshotError::InvalidMagic(hdr.magic)); - } - - let data: O = Self::deserialize(reader)?; - Ok((data, hdr.version)) - } - - /// Load a snapshot from a reader and validate its CRC - pub fn load(reader: &mut T, snapshot_len: usize) -> Result<(O, Version), SnapshotError> - where - T: Read + Debug, - O: DeserializeOwned + Debug, - { - let mut crc_reader = CRC64Reader::new(reader); - - // Fail-fast if the snapshot length is too small - let raw_snapshot_len = snapshot_len - .checked_sub(std::mem::size_of::()) - .ok_or(SnapshotError::InvalidSnapshotSize)?; - - // Read everything apart from the CRC. - let mut snapshot = vec![0u8; raw_snapshot_len]; - crc_reader - .read_exact(&mut snapshot) - .map_err(|ref err| SnapshotError::Io(err.raw_os_error().unwrap_or(libc::EINVAL)))?; - - // Since the reader updates the checksum as bytes ar being read from it, the order of these - // 2 statements is important, we first get the checksum computed on the read bytes - // then read the stored checksum. - let computed_checksum = crc_reader.checksum(); - let stored_checksum: u64 = Self::deserialize(&mut crc_reader)?; - if computed_checksum != stored_checksum { - return Err(SnapshotError::Crc64(computed_checksum)); - } - - let mut snapshot_slice: &[u8] = snapshot.as_mut_slice(); - Snapshot::unchecked_load::<_, O>(&mut snapshot_slice) - } - - /// Load a snapshot from a reader object and perform a snapshot version check - pub fn load_with_version_check( - &self, - reader: &mut T, - snapshot_len: usize, - ) -> Result - where - T: Read + Debug, - O: DeserializeOwned + Debug, - { - let (data, version) = Snapshot::load::<_, O>(reader, snapshot_len)?; - if version.major != self.version.major || version.minor > self.version.minor { - Err(SnapshotError::InvalidFormatVersion(version)) - } else { - Ok(data) - } - } - - /// Saves a snapshot and include a CRC64 checksum. - pub fn save(&self, writer: &mut T, object: &O) -> Result<(), SnapshotError> - where - T: Write + Debug, - O: Serialize + Debug, - { - let mut crc_writer = CRC64Writer::new(writer); - self.save_without_crc(&mut crc_writer, object)?; - - // Now write CRC value - let checksum = crc_writer.checksum(); - Self::serialize(&mut crc_writer, &checksum) - } - - /// Save a snapshot with no CRC64 checksum included. - pub fn save_without_crc( - &self, - mut writer: &mut T, - object: &O, - ) -> Result<(), SnapshotError> - where - T: Write, - O: Serialize + Debug, - { - // Write magic value and snapshot version - Self::serialize(&mut writer, &SnapshotHdr::new(self.version.clone()))?; - // Write data - Self::serialize(&mut writer, object) +impl Snapshot { + pub fn new(header: SnapshotHdr, data: Data) -> Self { + Snapshot { header, data } } } @@ -343,155 +217,152 @@ mod tests { #[test] fn test_parse_version_from_file() { - let snapshot = Snapshot::new(Version::new(1, 0, 42)); - // Enough memory for the header, 1 byte and the CRC let mut snapshot_data = vec![0u8; 100]; - snapshot - .save(&mut snapshot_data.as_mut_slice(), &42u8) - .unwrap(); + let snapshot = SnapshotHdr::new(Version::new(1, 0, 42)); + snapshot.store(&mut snapshot_data).unwrap(); assert_eq!( - Snapshot::get_format_version(&mut snapshot_data.as_slice()).unwrap(), + SnapshotHdr::load(&mut snapshot_data.as_slice()).unwrap().version, Version::new(1, 0, 42) ); } - #[test] - fn test_bad_snapshot_size() { - let snapshot_data = vec![0u8; 1]; - - let snapshot = Snapshot::new(Version::new(1, 6, 1)); - assert!(matches!( - snapshot.load_with_version_check::<_, u8>( - &mut snapshot_data.as_slice(), - snapshot_data.len() - ), - Err(SnapshotError::InvalidSnapshotSize) - )); - } - - #[test] - fn test_bad_reader() { - #[derive(Debug)] - struct BadReader; - - impl Read for BadReader { - fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { - Err(std::io::ErrorKind::InvalidInput.into()) - } - } - - let mut reader = BadReader {}; - - let snapshot = Snapshot::new(Version::new(42, 27, 18)); - assert!(matches!( - snapshot.load_with_version_check::<_, u8>(&mut reader, 1024), - Err(SnapshotError::Io(_)) - )); - } - - #[test] - fn test_bad_magic() { - let mut data = vec![0u8; 100]; - - let snapshot = Snapshot::new(Version::new(24, 16, 1)); - snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); - - // Writing dummy values in the first bytes of the snapshot data (we are on little-endian - // machines) should trigger an `Error::InvalidMagic` error. - data[0] = 0x01; - data[1] = 0x02; - data[2] = 0x03; - data[3] = 0x04; - data[4] = 0x42; - data[5] = 0x43; - data[6] = 0x44; - data[7] = 0x45; - assert!(matches!( - Snapshot::unchecked_load::<_, u8>(&mut data.as_slice()), - Err(SnapshotError::InvalidMagic(0x4544_4342_0403_0201u64)) - )); - } - - #[test] - fn test_bad_crc() { - let mut data = vec![0u8; 100]; - - let snapshot = Snapshot::new(Version::new(12, 1, 3)); - snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); - - // Tamper the bytes written, without touching the previously CRC. - snapshot - .save_without_crc(&mut data.as_mut_slice(), &43u8) - .unwrap(); - - assert!(matches!( - snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - Err(SnapshotError::Crc64(_)) - )); - } - - #[test] - fn test_bad_version() { - let mut data = vec![0u8; 100]; - - // We write a snapshot with version "v1.3.12" - let snapshot = Snapshot::new(Version::new(1, 3, 12)); - snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); - - // Different major versions should not work - let snapshot = Snapshot::new(Version::new(2, 3, 12)); - assert!(matches!( - snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - Err(SnapshotError::InvalidFormatVersion(Version { - major: 1, - minor: 3, - patch: 12, - .. - })) - )); - let snapshot = Snapshot::new(Version::new(0, 3, 12)); - assert!(matches!( - snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - Err(SnapshotError::InvalidFormatVersion(Version { - major: 1, - minor: 3, - patch: 12, - .. - })) - )); - - // We can't support minor versions bigger than ours - let snapshot = Snapshot::new(Version::new(1, 2, 12)); - assert!(matches!( - snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - Err(SnapshotError::InvalidFormatVersion(Version { - major: 1, - minor: 3, - patch: 12, - .. - })) - )); - - // But we can support minor versions smaller or equeal to ours. We also support - // all patch versions within our supported major.minor version. - let snapshot = Snapshot::new(Version::new(1, 4, 12)); - snapshot - .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - .unwrap(); - let snapshot = Snapshot::new(Version::new(1, 3, 0)); - snapshot - .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - .unwrap(); - let snapshot = Snapshot::new(Version::new(1, 3, 12)); - snapshot - .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - .unwrap(); - let snapshot = Snapshot::new(Version::new(1, 3, 1024)); - snapshot - .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - .unwrap(); - } + // #[test] + // fn test_bad_snapshot_size() { + // let snapshot_data = vec![0u8; 1]; + + // let snapshot = SnapshotHdr::new(Version::new(1, 6, 1)); + // assert!(matches!( + // snapshot.load_with_version_check::<_, u8>( + // &mut snapshot_data.as_slice(), + // snapshot_data.len() + // ), + // Err(SnapshotError::InvalidSnapshotSize) + // )); + // } + + // #[test] + // fn test_bad_reader() { + // #[derive(Debug)] + // struct BadReader; + + // impl Read for BadReader { + // fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { + // Err(std::io::ErrorKind::InvalidInput.into()) + // } + // } + + // let mut reader = BadReader {}; + + // let snapshot = Snapshot::new(Version::new(42, 27, 18)); + // assert!(matches!( + // snapshot.load_with_version_check::<_, u8>(&mut reader, 1024), + // Err(SnapshotError::Io(_)) + // )); + // } + + // #[test] + // fn test_bad_magic() { + // let mut data = vec![0u8; 100]; + + // let snapshot = Snapshot::new(Version::new(24, 16, 1)); + // snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); + + // // Writing dummy values in the first bytes of the snapshot data (we are on little-endian + // // machines) should trigger an `Error::InvalidMagic` error. + // data[0] = 0x01; + // data[1] = 0x02; + // data[2] = 0x03; + // data[3] = 0x04; + // data[4] = 0x42; + // data[5] = 0x43; + // data[6] = 0x44; + // data[7] = 0x45; + // assert!(matches!( + // Snapshot::unchecked_load::<_, u8>(&mut data.as_slice()), + // Err(SnapshotError::InvalidMagic(0x4544_4342_0403_0201u64)) + // )); + // } + + // #[test] + // fn test_bad_crc() { + // let mut data = vec![0u8; 100]; + + // let snapshot = Snapshot::new(Version::new(12, 1, 3)); + // snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); + + // // Tamper the bytes written, without touching the previously CRC. + // snapshot + // .save_without_crc(&mut data.as_mut_slice(), &43u8) + // .unwrap(); + + // assert!(matches!( + // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), + // Err(SnapshotError::Crc64(_)) + // )); + // } + + // #[test] + // fn test_bad_version() { + // let mut data = vec![0u8; 100]; + + // // We write a snapshot with version "v1.3.12" + // let snapshot = Snapshot::new(Version::new(1, 3, 12)); + // snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); + + // // Different major versions should not work + // let snapshot = Snapshot::new(Version::new(2, 3, 12)); + // assert!(matches!( + // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), + // Err(SnapshotError::InvalidFormatVersion(Version { + // major: 1, + // minor: 3, + // patch: 12, + // .. + // })) + // )); + // let snapshot = Snapshot::new(Version::new(0, 3, 12)); + // assert!(matches!( + // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), + // Err(SnapshotError::InvalidFormatVersion(Version { + // major: 1, + // minor: 3, + // patch: 12, + // .. + // })) + // )); + + // // We can't support minor versions bigger than ours + // let snapshot = Snapshot::new(Version::new(1, 2, 12)); + // assert!(matches!( + // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), + // Err(SnapshotError::InvalidFormatVersion(Version { + // major: 1, + // minor: 3, + // patch: 12, + // .. + // })) + // )); + + // // But we can support minor versions smaller or equeal to ours. We also support + // // all patch versions within our supported major.minor version. + // let snapshot = Snapshot::new(Version::new(1, 4, 12)); + // snapshot + // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) + // .unwrap(); + // let snapshot = Snapshot::new(Version::new(1, 3, 0)); + // snapshot + // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) + // .unwrap(); + // let snapshot = Snapshot::new(Version::new(1, 3, 12)); + // snapshot + // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) + // .unwrap(); + // let snapshot = Snapshot::new(Version::new(1, 3, 1024)); + // snapshot + // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) + // .unwrap(); + // } } From 86eae9ccd8e0adf0123c16893b904be443e8856c Mon Sep 17 00:00:00 2001 From: Bhavesh M Date: Sat, 20 Jul 2024 00:57:51 +0000 Subject: [PATCH 3/8] Fix issues with `src/snapshot-editor/src/utils.rs` Signed-off-by: Gabriel Keller --- src/snapshot-editor/src/utils.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/snapshot-editor/src/utils.rs b/src/snapshot-editor/src/utils.rs index 3bccf46917b..4036f8e2078 100644 --- a/src/snapshot-editor/src/utils.rs +++ b/src/snapshot-editor/src/utils.rs @@ -7,7 +7,7 @@ use std::path::PathBuf; use fc_utils::u64_to_usize; use semver::Version; use vmm::persist::MicrovmState; -use vmm::snapshot::Snapshot; +use vmm::snapshot::{Snapshot, SnapshotError, SnapshotHdr}; // Some errors are only used in aarch64 code #[allow(unused)] @@ -26,11 +26,13 @@ pub enum UtilsError { } #[allow(unused)] -pub fn open_vmstate(snapshot_path: &PathBuf) -> Result<(MicrovmState, Version), UtilsError> { +pub fn open_vmstate(snapshot_path: &PathBuf) -> Result, UtilsError> { let mut snapshot_reader = File::open(snapshot_path).map_err(UtilsError::VmStateFileOpen)?; let metadata = std::fs::metadata(snapshot_path).map_err(UtilsError::VmStateFileMeta)?; let snapshot_len = u64_to_usize(metadata.len()); - Snapshot::load(&mut snapshot_reader, snapshot_len).map_err(UtilsError::VmStateLoad) + + let snapshot: Result, UtilsError> = Snapshot::load(&mut snapshot_reader, snapshot_len).map_err(UtilsError::VmStateLoad); + snapshot } // This method is used only in aarch64 code so far @@ -46,9 +48,10 @@ pub fn save_vmstate( .truncate(true) .open(output_path) .map_err(UtilsError::OutputFileOpen)?; - let mut snapshot = Snapshot::new(version); + let snapshot_hdr = SnapshotHdr::new(version); + let snapshot = Snapshot::new(snapshot_hdr, microvm_state); snapshot - .save(&mut output_file, µvm_state) + .save(&mut output_file) .map_err(UtilsError::VmStateSave)?; Ok(()) } From af2de998769e285c9bc452588c75d15c466155be Mon Sep 17 00:00:00 2001 From: Bhavesh M Date: Sat, 20 Jul 2024 01:10:45 +0000 Subject: [PATCH 4/8] Temporarily comment out part of `print_snapshot_data_format` Signed-off-by: Gabriel Keller --- src/firecracker/src/main.rs | 7 ++++--- src/vmm/src/snapshot/mod.rs | 9 ++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/firecracker/src/main.rs b/src/firecracker/src/main.rs index f353ea26bb4..356368207a6 100644 --- a/src/firecracker/src/main.rs +++ b/src/firecracker/src/main.rs @@ -539,10 +539,11 @@ fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersion let mut snapshot_reader = File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?; - let data_format_version = Snapshot::get_format_version(&mut snapshot_reader) - .map_err(SnapshotVersionError::SnapshotVersion)?; + // TODO: Need to fix this + // let data_format_version = Snapshot::get_format_version(&mut snapshot_reader) + // .map_err(SnapshotVersionError::SnapshotVersion)?; - println!("v{}", data_format_version); + // println!("v{}", data_format_version); Ok(()) } diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index f743cc12197..e1b50101247 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -132,10 +132,7 @@ impl Deserialize<'a>> Snapshot { } let data: Data = Self::deserialize(reader)?; - Ok(Self { - header: hdr, - data, - }) + Ok(Self { header: hdr, data }) } pub fn load(reader: &mut R, snapshot_len: usize) -> Result @@ -224,7 +221,9 @@ mod tests { snapshot.store(&mut snapshot_data).unwrap(); assert_eq!( - SnapshotHdr::load(&mut snapshot_data.as_slice()).unwrap().version, + SnapshotHdr::load(&mut snapshot_data.as_slice()) + .unwrap() + .version, Version::new(1, 0, 42) ); } From bfc8c480235425deca9fdd43f7f1cb249e7c97cd Mon Sep 17 00:00:00 2001 From: Bhavesh M Date: Thu, 17 Oct 2024 17:25:09 -0500 Subject: [PATCH 5/8] Signed-off-by: Gabriel Keller --- src/snapshot-editor/src/utils.rs | 13 +++- src/vmm/src/persist.rs | 6 +- src/vmm/src/snapshot/mod.rs | 99 ++++++++++++++------------- src/vmm/src/vmm_config/boot_source.rs | 2 +- src/vmm/src/vstate/memory.rs | 2 +- src/vmm/src/vstate/vm.rs | 2 +- 6 files changed, 68 insertions(+), 56 deletions(-) diff --git a/src/snapshot-editor/src/utils.rs b/src/snapshot-editor/src/utils.rs index 4036f8e2078..ccf230e107f 100644 --- a/src/snapshot-editor/src/utils.rs +++ b/src/snapshot-editor/src/utils.rs @@ -26,13 +26,22 @@ pub enum UtilsError { } #[allow(unused)] -pub fn open_vmstate(snapshot_path: &PathBuf) -> Result, UtilsError> { +pub fn open_vmstate(snapshot_path: &PathBuf) -> Result<(MicrovmState, Version), UtilsError> { let mut snapshot_reader = File::open(snapshot_path).map_err(UtilsError::VmStateFileOpen)?; let metadata = std::fs::metadata(snapshot_path).map_err(UtilsError::VmStateFileMeta)?; let snapshot_len = u64_to_usize(metadata.len()); let snapshot: Result, UtilsError> = Snapshot::load(&mut snapshot_reader, snapshot_len).map_err(UtilsError::VmStateLoad); - snapshot + match snapshot { + Ok(snapshot) => { + let version = snapshot.version(); + Ok((snapshot.data, version.to_owned())) + } + Err(e) => { + return Err(e); + } + } + } // This method is used only in aarch64 code so far diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs index edd0e5e900d..f24eac1968a 100644 --- a/src/vmm/src/persist.rs +++ b/src/vmm/src/persist.rs @@ -731,6 +731,8 @@ mod tests { #[cfg(target_arch = "x86_64")] acpi_dev_state: vmm.acpi_device_manager.save(), }; + let vm_info = microvm_state.vm_info.clone(); + let device_states = microvm_state.device_states.clone(); let mut buf = vec![0; 10000]; @@ -742,10 +744,10 @@ mod tests { Snapshot::load(&mut buf.as_slice(), buf.len()).unwrap(); let restored_microvm_state = restored_snapshot.data; - assert_eq!(restored_microvm_state.vm_info, microvm_state.vm_info); + assert_eq!(restored_microvm_state.vm_info, vm_info); assert_eq!( restored_microvm_state.device_states, - microvm_state.device_states + device_states ) } diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index e1b50101247..aa02e6cefa5 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -82,19 +82,16 @@ impl SnapshotHdr { } pub fn load(reader: &mut R) -> Result { - let hdr: SnapshotHdr = bincode::DefaultOptions::new() - .with_limit(VM_STATE_DESERIALIZE_LIMIT) - .with_fixint_encoding() - .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after - // reading the header, there will be trailing bytes. - .deserialize_from(reader) - .map_err(|err| SnapshotError::Serde(err.to_string()))?; + let hdr: SnapshotHdr = deserialize(reader)?; Ok(hdr) } pub fn store(&self, writer: &mut W) -> Result<(), SnapshotError> { - bincode::serialize_into(writer, self).map_err(|err| SnapshotError::Serde(err.to_string())) + match serialize(writer, self) { + Ok(_) => Ok(()), + Err(e) => Err(e) + } } } @@ -105,33 +102,17 @@ pub struct Snapshot { pub data: Data, } -impl Deserialize<'a>> Snapshot { - /// Helper function to deserialize an object from a reader - pub fn deserialize(reader: &mut T) -> Result - where - T: Read, - O: DeserializeOwned + Debug, - { - // flags below are those used by default by bincode::deserialize_from, plus `with_limit`. - bincode::DefaultOptions::new() - .with_limit(VM_STATE_DESERIALIZE_LIMIT) - .with_fixint_encoding() - .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after - // reading the header, there will be trailing bytes. - .deserialize_from(reader) - .map_err(|err| SnapshotError::Serde(err.to_string())) - } - +impl Snapshot { pub fn load_unchecked(reader: &mut R) -> Result where Data: DeserializeOwned + Debug, { - let hdr: SnapshotHdr = Self::deserialize(reader)?; + let hdr: SnapshotHdr = deserialize(reader)?; if hdr.magic != SNAPSHOT_MAGIC_ID { return Err(SnapshotError::InvalidMagic(hdr.magic)); } - let data: Data = Self::deserialize(reader)?; + let data: Data = deserialize(reader)?; Ok(Self { header: hdr, data }) } @@ -156,7 +137,7 @@ impl Deserialize<'a>> Snapshot { // 2 statements is important, we first get the checksum computed on the read bytes // then read the stored checksum. let computed_checksum = crc_reader.checksum(); - let stored_checksum: u64 = Self::deserialize(&mut crc_reader)?; + let stored_checksum: u64 = deserialize(&mut crc_reader)?; if computed_checksum != stored_checksum { return Err(SnapshotError::Crc64(computed_checksum)); } @@ -167,29 +148,11 @@ impl Deserialize<'a>> Snapshot { } impl Snapshot { - /// Helper function to serialize an object to a writer - pub fn serialize(writer: &mut T, data: &O) -> Result - where - T: Write, - O: Serialize + Debug, - { - let mut buffer = Vec::new(); - bincode::serialize_into(&mut buffer, data) - .map_err(|err| SnapshotError::Serde(err.to_string()))?; - - writer - .write_all(&buffer) - .map_err(|err| SnapshotError::Serde(err.to_string()))?; - - Ok(buffer.len()) - // bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) - } - pub fn save(&self, mut writer: &mut W) -> Result { // Write magic value and snapshot version - Self::serialize(&mut writer, &SnapshotHdr::new(self.header.version.clone()))?; + serialize(&mut writer, &SnapshotHdr::new(self.header.version.clone()))?; // Write data - Self::serialize(&mut writer, &self.data) + serialize(&mut writer, &self.data) } pub fn save_with_crc(&self, writer: &mut W) -> Result { @@ -198,7 +161,7 @@ impl Snapshot { // Now write CRC value let checksum = crc_writer.checksum(); - Self::serialize(&mut crc_writer, &checksum) + serialize(&mut crc_writer, &checksum) } } @@ -206,6 +169,44 @@ impl Snapshot { pub fn new(header: SnapshotHdr, data: Data) -> Self { Snapshot { header, data } } + + pub fn version(&self) -> Version { + self.header.version.clone() + } +} + +/// Helper function to deserialize an object from a reader +fn deserialize(reader: &mut T) -> Result +where + T: Read, + O: DeserializeOwned + Debug, +{ + // flags below are those used by default by bincode::deserialize_from, plus `with_limit`. + bincode::DefaultOptions::new() + .with_limit(VM_STATE_DESERIALIZE_LIMIT) + .with_fixint_encoding() + .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after + // reading the header, there will be trailing bytes. + .deserialize_from(reader) + .map_err(|err| SnapshotError::Serde(err.to_string())) +} + +/// Helper function to serialize an object to a writer +fn serialize(writer: &mut T, data: &O) -> Result +where + T: Write, + O: Serialize + Debug, +{ + let mut buffer = Vec::new(); + bincode::serialize_into(&mut buffer, data) + .map_err(|err| SnapshotError::Serde(err.to_string()))?; + + writer + .write_all(&buffer) + .map_err(|err| SnapshotError::Serde(err.to_string()))?; + + Ok(buffer.len()) + // bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) } #[cfg(test)] diff --git a/src/vmm/src/vmm_config/boot_source.rs b/src/vmm/src/vmm_config/boot_source.rs index 8374ae335a8..c0a5c082068 100644 --- a/src/vmm/src/vmm_config/boot_source.rs +++ b/src/vmm/src/vmm_config/boot_source.rs @@ -132,7 +132,7 @@ pub(crate) mod tests { }; let mut snapshot_data = vec![0u8; 1000]; - Snapshot::serialize(&mut snapshot_data.as_mut_slice(), &boot_src_cfg).unwrap(); + Snapshot::serialize::<&BootSourceConfig>(&mut snapshot_data.as_mut_slice(), &boot_src_cfg).unwrap(); let restored_boot_cfg = Snapshot::deserialize(&mut snapshot_data.as_slice()).unwrap(); assert_eq!(boot_src_cfg, restored_boot_cfg); } diff --git a/src/vmm/src/vstate/memory.rs b/src/vmm/src/vstate/memory.rs index 5f0390ac72d..60b225242f5 100644 --- a/src/vmm/src/vstate/memory.rs +++ b/src/vmm/src/vstate/memory.rs @@ -603,7 +603,7 @@ mod tests { fn check_serde(guest_memory: &GuestMemoryMmap) { let mut snapshot_data = vec![0u8; 10000]; let original_state = guest_memory.describe(); - Snapshot::serialize(&mut snapshot_data.as_mut_slice(), &original_state).unwrap(); + Snapshot::serialize::(&mut snapshot_data.as_mut_slice(), &original_state).unwrap(); let restored_state = Snapshot::deserialize(&mut snapshot_data.as_slice()).unwrap(); assert_eq!(original_state, restored_state); } diff --git a/src/vmm/src/vstate/vm.rs b/src/vmm/src/vstate/vm.rs index 55b0ec146e0..333bf80c158 100644 --- a/src/vmm/src/vstate/vm.rs +++ b/src/vmm/src/vstate/vm.rs @@ -579,7 +579,7 @@ pub(crate) mod tests { let (mut vm, _) = setup_vm(0x1000); vm.setup_irqchip().unwrap(); let state = vm.save_state().unwrap(); - Snapshot::serialize(&mut snapshot_data.as_mut_slice(), &state).unwrap(); + Snapshot::serialize::<&mut [u8], VmState>(&mut snapshot_data.as_mut_slice(), &state).unwrap(); let restored_state: VmState = Snapshot::deserialize(&mut snapshot_data.as_slice()).unwrap(); vm.restore_state(&restored_state).unwrap(); From 3252af5c9bff66dda514ee024f964ce9a574ab4c Mon Sep 17 00:00:00 2001 From: Gabriel Keller Date: Wed, 7 May 2025 03:52:14 +0000 Subject: [PATCH 6/8] Snapshot refactoring Signed-off-by: Gabriel Keller --- src/vmm/src/snapshot/mod.rs | 153 ++++++++++++++++++++---------------- 1 file changed, 87 insertions(+), 66 deletions(-) diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index f68059288f1..bd8cbd48c4f 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -30,6 +30,7 @@ use std::io::{Read, Write}; use bincode::config; use bincode::config::{Configuration, Fixint, Limit, LittleEndian}; +use bincode::error::{DecodeError, EncodeError}; use semver::Version; use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; @@ -72,35 +73,37 @@ pub enum SnapshotError { /// Firecracker snapshot header #[derive(Debug, Serialize, Deserialize)] -pub struct SnapshotHdr { - /// magic value +struct SnapshotHdr { + /// Magic value magic: u64, /// Snapshot data version version: Version, } impl SnapshotHdr { - pub fn new(version: Version) -> Self { + /// Create a new header for writing snapshots + fn new(version: Version) -> Self { Self { magic: SNAPSHOT_MAGIC_ID, version, } } - pub fn load(reader: &mut R) -> Result { + /// Load and deserialize just the header (magic + version) + fn load(reader: &mut R) -> Result { let hdr: SnapshotHdr = deserialize(reader)?; - - Ok(hdr) + if hdr.magic != SNAPSHOT_MAGIC_ID { + Err(SnapshotError::InvalidMagic(hdr.magic)) + } + else { + Ok(hdr) + } } - /// Helper function to deserialize an object from a reader - pub fn deserialize(reader: &mut T) -> Result - where - T: Read, - O: DeserializeOwned + Debug, - { - bincode::serde::decode_from_std_read(reader, BINCODE_CONFIG) - .map_err(|err| SnapshotError::Serde(err.to_string())) + /// Serialize and write just the header + fn store(&self, writer: &mut W) -> Result<(), SnapshotError> { + serialize(writer, self)?; + Ok(()) } } @@ -116,51 +119,44 @@ impl SnapshotHdr { Ok(()) } -impl Snapshot { - pub fn load_unchecked(reader: &mut R) -> Result - where - Data: DeserializeOwned + Debug, - { - let hdr: SnapshotHdr = deserialize(reader)?; - if hdr.magic != SNAPSHOT_MAGIC_ID { - return Err(SnapshotError::InvalidMagic(hdr.magic)); - } - - let data: Data = deserialize(reader)?; +// Implementations for deserializing snapshots +// Publicly exposed functions: +// - load_unchecked() +//- load() +impl Snapshot { + /// Load without CRC or version‐check, but verify magic via `SnapshotHdr::load`. + pub fn load_unchecked(reader: &mut R) -> Result { + // this calls `deserialize` + checks magic internally + let hdr: SnapshotHdr = SnapshotHdr::load(reader)?; + let data: Data = deserialize(reader)?; Ok(Self { header: hdr, data }) } - pub fn load(reader: &mut R, snapshot_len: usize) -> Result - where - Data: DeserializeOwned + Debug, - { + /// Load with CRC64 validation in one pass, using `load_unchecked` for header+data. + pub fn load(reader: &mut R) -> Result { + // 1) Wrap in CRC reader let mut crc_reader = CRC64Reader::new(reader); - // Fail-fast if the snapshot length is too small - let raw_snapshot_len = snapshot_len - .checked_sub(std::mem::size_of::()) - .ok_or(SnapshotError::InvalidSnapshotSize)?; - - // Read everything apart from the CRC. - let mut snapshot = vec![0u8; raw_snapshot_len]; - crc_reader - .read_exact(&mut snapshot) - .map_err(|ref err| SnapshotError::Io(err.raw_os_error().unwrap_or(libc::EINVAL)))?; - - // Since the reader updates the checksum as bytes ar being read from it, the order of these - // 2 statements is important, we first get the checksum computed on the read bytes - // then read the stored checksum. - let computed_checksum = crc_reader.checksum(); - let stored_checksum: u64 = deserialize(&mut crc_reader)?; - if computed_checksum != stored_checksum { - return Err(SnapshotError::Crc64(computed_checksum)); + // 2) Parse header + payload & magic‐check + let snapshot = Snapshot::load_unchecked(&mut crc_reader)?; + + // 3) Grab the computed CRC over everything read so far + let computed = crc_reader.checksum(); + + // 4) Deserialize the trailing u64 and compare + let stored: u64 = deserialize(&mut crc_reader)?; + if stored != computed { + return Err(SnapshotError::Crc64(computed)); } - let mut snapshot_slice: &[u8] = snapshot.as_mut_slice(); - Snapshot::load_unchecked::<_>(&mut snapshot_slice) + Ok(snapshot) } } +// Implementations for serializing snapshots +// Publicly-exposed *methods*: +// - save(self,...) +// - save_with_crc(self,...) impl Snapshot { pub fn save(&self, mut writer: &mut W) -> Result { // Write magic value and snapshot version @@ -179,8 +175,12 @@ impl Snapshot { } } +// General methods for snapshots (related to serialization, see above, since an +// instance is needed to serialize) impl Snapshot { - pub fn new(header: SnapshotHdr, data: Data) -> Self { + /// Construct from a pre‐built header + payload + pub fn new(version: Version, data: Data) -> Self { + header = SnapshotHdr::new(version); Snapshot { header, data } } @@ -189,35 +189,56 @@ impl Snapshot { } } -/// Helper function to deserialize an object from a reader +/// Deserialize any `O: DeserializeOwned + Debug` via bincode + our config, fn deserialize(reader: &mut T) -> Result where T: Read, O: DeserializeOwned + Debug, { - // flags below are those used by default by bincode::deserialize_from, plus `with_limit`. - bincode::DefaultOptions::new() - .with_limit(VM_STATE_DESERIALIZE_LIMIT) - .with_fixint_encoding() - .allow_trailing_bytes() // need this because we deserialize header and snapshot from the same file, so after - // reading the header, there will be trailing bytes. - .deserialize_from(reader) - .map_err(|err| SnapshotError::Serde(err.to_string())) + bincode::serde::decode_from_std_read(reader, BINCODE_CONFIG) + .map_err(|err| match err { + // The reader hit an actual IO error. + DecodeError::Io { inner, .. } => + SnapshotError::Io(inner.raw_os_error().unwrap_or(EIO)), + + // Not enough bytes in the input for what we expected. + DecodeError::UnexpectedEnd { .. } | + DecodeError::LimitExceeded => + SnapshotError::InvalidSnapshotSize, + + // Anything else is a ser/de format issue. + other => + SnapshotError::Serde(other.to_string()), + }) } -/// Helper function to serialize an object to a writer +/// Serialize any `O: Serialize + Debug` into a Vec, write it, and return the byte‐count, fn serialize(writer: &mut T, data: &O) -> Result where T: Write, O: Serialize + Debug, { - let mut buffer = Vec::new(); - bincode::serialize_into(&mut buffer, data) - .map_err(|err| SnapshotError::Serde(err.to_string()))?; - + // 1) Encode into an in-memory buffer + let mut buf = Vec::new(); + bincode::serde::encode_into_std_write(data, &mut buf, BINCODE_CONFIG) + .map_err(|err| match err { + // Ran out of room while encoding + EncodeError::UnexpectedEnd => + SnapshotError::Io(libc::EIO), + + // Underlying IO failure during encode (index tells how many bytes got written) + EncodeError::Io { inner, .. } => + SnapshotError::Io(inner.raw_os_error().unwrap_or(libc::EIO)), + + // Any other encode error we surface as Serde + other => + SnapshotError::Serde(other.to_string()), + })?; + + // 2) Flush that buffer to the target writer writer - .write_all(&buffer) - .map_err(|err| SnapshotError::Serde(err.to_string()))?; + .write_all(&buf) + .map_err(|io_err| SnapshotError::Io(io_err.raw_os_error().unwrap_or(libc::EIO)))?; Ok(buffer.len()) // bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) From 5036ba7f047850d2c847ca599a59a663994391c4 Mon Sep 17 00:00:00 2001 From: Gabriel Keller Date: Wed, 7 May 2025 07:16:32 +0000 Subject: [PATCH 7/8] Uncomment tests and change tests to new system Signed-off-by: Gabriel Keller --- src/vmm/src/snapshot/mod.rs | 209 +++++++++++++----------------------- 1 file changed, 73 insertions(+), 136 deletions(-) diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index bd8cbd48c4f..0d9fbdd4102 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -264,140 +264,77 @@ mod tests { ); } - // #[test] - // fn test_bad_snapshot_size() { - // let snapshot_data = vec![0u8; 1]; - - // let snapshot = SnapshotHdr::new(Version::new(1, 6, 1)); - // assert!(matches!( - // snapshot.load_with_version_check::<_, u8>( - // &mut snapshot_data.as_slice(), - // snapshot_data.len() - // ), - // Err(SnapshotError::InvalidSnapshotSize) - // )); - // } - - // #[test] - // fn test_bad_reader() { - // #[derive(Debug)] - // struct BadReader; - - // impl Read for BadReader { - // fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { - // Err(std::io::ErrorKind::InvalidInput.into()) - // } - // } - - // let mut reader = BadReader {}; - - // let snapshot = Snapshot::new(Version::new(42, 27, 18)); - // assert!(matches!( - // snapshot.load_with_version_check::<_, u8>(&mut reader, 1024), - // Err(SnapshotError::Io(_)) - // )); - // } - - // #[test] - // fn test_bad_magic() { - // let mut data = vec![0u8; 100]; - - // let snapshot = Snapshot::new(Version::new(24, 16, 1)); - // snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); - - // // Writing dummy values in the first bytes of the snapshot data (we are on little-endian - // // machines) should trigger an `Error::InvalidMagic` error. - // data[0] = 0x01; - // data[1] = 0x02; - // data[2] = 0x03; - // data[3] = 0x04; - // data[4] = 0x42; - // data[5] = 0x43; - // data[6] = 0x44; - // data[7] = 0x45; - // assert!(matches!( - // Snapshot::unchecked_load::<_, u8>(&mut data.as_slice()), - // Err(SnapshotError::InvalidMagic(0x4544_4342_0403_0201u64)) - // )); - // } - - // #[test] - // fn test_bad_crc() { - // let mut data = vec![0u8; 100]; - - // let snapshot = Snapshot::new(Version::new(12, 1, 3)); - // snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); - - // // Tamper the bytes written, without touching the previously CRC. - // snapshot - // .save_without_crc(&mut data.as_mut_slice(), &43u8) - // .unwrap(); - - // assert!(matches!( - // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - // Err(SnapshotError::Crc64(_)) - // )); - // } - - // #[test] - // fn test_bad_version() { - // let mut data = vec![0u8; 100]; - - // // We write a snapshot with version "v1.3.12" - // let snapshot = Snapshot::new(Version::new(1, 3, 12)); - // snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); - - // // Different major versions should not work - // let snapshot = Snapshot::new(Version::new(2, 3, 12)); - // assert!(matches!( - // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - // Err(SnapshotError::InvalidFormatVersion(Version { - // major: 1, - // minor: 3, - // patch: 12, - // .. - // })) - // )); - // let snapshot = Snapshot::new(Version::new(0, 3, 12)); - // assert!(matches!( - // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - // Err(SnapshotError::InvalidFormatVersion(Version { - // major: 1, - // minor: 3, - // patch: 12, - // .. - // })) - // )); - - // // We can't support minor versions bigger than ours - // let snapshot = Snapshot::new(Version::new(1, 2, 12)); - // assert!(matches!( - // snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()), - // Err(SnapshotError::InvalidFormatVersion(Version { - // major: 1, - // minor: 3, - // patch: 12, - // .. - // })) - // )); - - // // But we can support minor versions smaller or equeal to ours. We also support - // // all patch versions within our supported major.minor version. - // let snapshot = Snapshot::new(Version::new(1, 4, 12)); - // snapshot - // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - // .unwrap(); - // let snapshot = Snapshot::new(Version::new(1, 3, 0)); - // snapshot - // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - // .unwrap(); - // let snapshot = Snapshot::new(Version::new(1, 3, 12)); - // snapshot - // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - // .unwrap(); - // let snapshot = Snapshot::new(Version::new(1, 3, 1024)); - // snapshot - // .load_with_version_check::<_, u8>(&mut data.as_slice(), data.len()) - // .unwrap(); - // } + #[test] + fn test_bad_snapshot_size() { + let snapshot_data = vec![0u8; 1]; + + let snapshot = SnapshotHdr::new(Version::new(1, 6, 1)); + assert!(matches!( + Snapshot::load::<_, u8>( + &mut snapshot_data.as_slice(), + ), + Err(SnapshotError::InvalidSnapshotSize) + )); + } + + #[test] + fn test_bad_reader() { + #[derive(Debug)] + struct BadReader; + + impl Read for BadReader { + fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { + Err(std::io::ErrorKind::InvalidInput.into()) + } + } + + let mut reader = BadReader {}; + + assert!(matches!( + Snapshot::load::<_, u8>(&mut reader), + Err(SnapshotError::Io(_)) + )); + } + + #[test] + fn test_bad_magic() { + let mut data = vec![0u8; 100]; + + let snapshot = Snapshot::new(Version::new(24, 16, 1), &42u8); + snapshot.save(&mut data.as_mut_slice()).unwrap(); + + // Writing dummy values in the first bytes of the snapshot data (we are on little-endian + // machines) should trigger an `Error::InvalidMagic` error. + data[0] = 0x01; + data[1] = 0x02; + data[2] = 0x03; + data[3] = 0x04; + data[4] = 0x42; + data[5] = 0x43; + data[6] = 0x44; + data[7] = 0x45; + assert!(matches!( + Snapshot::unchecked_load::<_, u8>(&mut data.as_slice()), + Err(SnapshotError::InvalidMagic(0x4544_4342_0403_0201u64)) + )); + } + + #[test] + fn test_bad_crc() { + let mut data = vec![0u8; 100]; + + let snapshot = Snapshot::new(Version::new(12, 1, 3), &42u8); + snapshot.save(&mut data.as_mut_slice()).unwrap(); + + // Tamper the bytes written, without touching the previously CRC. + let snapshot2 = Snapshot::new(Version::new(12, 1, 3), &43u8); + snapshot2 + .save_without_crc(&mut data.as_mut_slice()) + .unwrap(); + + assert!(matches!( + Snapshot::load::<_, u8>(&mut data.as_slice()), + Err(SnapshotError::Crc64(_)) + )); + } } From 0acdb7cb6fe20e2274d3ce54b670199cdf09bf25 Mon Sep 17 00:00:00 2001 From: Gabriel Keller Date: Wed, 7 May 2025 08:44:08 +0000 Subject: [PATCH 8/8] Add back load_with_version_check and test, add descriptive errors, fix uses of snapshot Signed-off-by: Gabriel Keller --- src/firecracker/src/main.rs | 14 ++-- src/vmm/src/persist.rs | 9 +-- src/vmm/src/snapshot/mod.rs | 147 ++++++++++++++++++++++++++---------- 3 files changed, 115 insertions(+), 55 deletions(-) diff --git a/src/firecracker/src/main.rs b/src/firecracker/src/main.rs index 3a84e982baf..4f1763985e3 100644 --- a/src/firecracker/src/main.rs +++ b/src/firecracker/src/main.rs @@ -529,20 +529,18 @@ fn warn_deprecated_parameters() {} enum SnapshotVersionError { /// Unable to open snapshot state file: {0} OpenSnapshot(io::Error), - /// Invalid data format version of snapshot file: {0} - SnapshotVersion(SnapshotError), + /// Invalid data format of snapshot file: {0} + LoadSnapshot(SnapshotError), } // Print data format of provided snapshot state file. fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersionError> { - let mut snapshot_reader = - File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?; + let mut snapshot_reader = File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot); - // TODO: Need to fix this - // let data_format_version = Snapshot::get_format_version(&mut snapshot_reader) - // .map_err(SnapshotVersionError::SnapshotVersion)?; + let snapshot = Snapshot::load(snapshot_reader).map_err(); + let data_format_version = snapshot.version(SnapshotVersionError::LoadSnapshot); - // println!("v{}", data_format_version); + println!("v{}", data_format_version); Ok(()) } diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs index e63914c5eb9..7cd3d2da82b 100644 --- a/src/vmm/src/persist.rs +++ b/src/vmm/src/persist.rs @@ -427,9 +427,7 @@ fn snapshot_state_from_file( ) -> Result { let mut snapshot_reader = File::open(snapshot_path).map_err(SnapshotStateFromFileError::Open)?; - let metadata = std::fs::metadata(snapshot_path).map_err(SnapshotStateFromFileError::Meta)?; - let snapshot_len = u64_to_usize(metadata.len()); - let state: Snapshot = Snapshot::load(&mut snapshot_reader, snapshot_len) + let state: Snapshot = Snapshot::load_with_version_check(&mut snapshot_reader, SNAPSHOT_VERSION) .map_err(SnapshotStateFromFileError::Load)?; Ok(state.data) @@ -690,12 +688,11 @@ mod tests { let mut buf = vec![0; 10000]; - let snapshot_hdr = SnapshotHdr::new(Version::new(1, 0, 42)); - let snapshot = Snapshot::new(snapshot_hdr, microvm_state); + let snapshot = Snapshot::new(Version::new(1, 0, 42), microvm_state); snapshot.save(&mut buf.as_mut_slice()).unwrap(); let restored_snapshot: Snapshot = - Snapshot::load(&mut buf.as_slice(), buf.len()).unwrap(); + Snapshot::load(&mut buf.as_slice()); let restored_microvm_state = restored_snapshot.data; assert_eq!(restored_microvm_state.vm_info, vm_info); diff --git a/src/vmm/src/snapshot/mod.rs b/src/vmm/src/snapshot/mod.rs index 0d9fbdd4102..8b660adc586 100644 --- a/src/vmm/src/snapshot/mod.rs +++ b/src/vmm/src/snapshot/mod.rs @@ -94,8 +94,7 @@ impl SnapshotHdr { let hdr: SnapshotHdr = deserialize(reader)?; if hdr.magic != SNAPSHOT_MAGIC_ID { Err(SnapshotError::InvalidMagic(hdr.magic)) - } - else { + } else { Ok(hdr) } } @@ -107,17 +106,17 @@ impl SnapshotHdr { } } - /// Helper function to serialize an object to a writer - pub fn serialize(writer: &mut T, data: &O) -> Result<(), SnapshotError> - where - T: Write, - O: Serialize + Debug, - { - bincode::serde::encode_into_std_write(data, writer, BINCODE_CONFIG) - .map_err(|err| SnapshotError::Serde(err.to_string()))?; +/// Helper function to serialize an object to a writer +pub fn serialize(writer: &mut T, data: &O) -> Result<(), SnapshotError> +where + T: Write, + O: Serialize + Debug, +{ + bincode::serde::encode_into_std_write(data, writer, BINCODE_CONFIG) + .map_err(|err| SnapshotError::Serde(err.to_string()))?; - Ok(()) - } + Ok(()) +} // Implementations for deserializing snapshots // Publicly exposed functions: @@ -128,11 +127,11 @@ impl Snapshot { pub fn load_unchecked(reader: &mut R) -> Result { // this calls `deserialize` + checks magic internally let hdr: SnapshotHdr = SnapshotHdr::load(reader)?; - let data: Data = deserialize(reader)?; + let data: Data = deserialize(reader)?; Ok(Self { header: hdr, data }) } - /// Load with CRC64 validation in one pass, using `load_unchecked` for header+data. + /// Load with CRC64 validation pub fn load(reader: &mut R) -> Result { // 1) Wrap in CRC reader let mut crc_reader = CRC64Reader::new(reader); @@ -151,6 +150,19 @@ impl Snapshot { Ok(snapshot) } + + /// Load with CRC64 validation, and check that snapshot against the specified version + pub fn load_with_verison_check( + reader: &mut R, + version: &Version, + ) -> Result { + Self = load(reader)?; + if Self.version.major != version.major || Self.version.minor > version.minor { + Err(SnapshotError::InvalidFormatVersion(Self.version)) + } else { + Ok(data) + } + } } // Implementations for serializing snapshots @@ -195,21 +207,18 @@ where T: Read, O: DeserializeOwned + Debug, { - bincode::serde::decode_from_std_read(reader, BINCODE_CONFIG) - .map_err(|err| match err { - // The reader hit an actual IO error. - DecodeError::Io { inner, .. } => - SnapshotError::Io(inner.raw_os_error().unwrap_or(EIO)), - - // Not enough bytes in the input for what we expected. - DecodeError::UnexpectedEnd { .. } | - DecodeError::LimitExceeded => - SnapshotError::InvalidSnapshotSize, - - // Anything else is a ser/de format issue. - other => - SnapshotError::Serde(other.to_string()), - }) + bincode::serde::decode_from_std_read(reader, BINCODE_CONFIG).map_err(|err| match err { + // The reader hit an actual IO error. + DecodeError::Io { inner, .. } => SnapshotError::Io(inner.raw_os_error().unwrap_or(EIO)), + + // Not enough bytes in the input for what we expected. + DecodeError::UnexpectedEnd { .. } | DecodeError::LimitExceeded => { + SnapshotError::InvalidSnapshotSize + } + + // Anything else is a ser/de format issue. + other => SnapshotError::Serde(other.to_string()), + }) } /// Serialize any `O: Serialize + Debug` into a Vec, write it, and return the byte‐count, @@ -220,20 +229,20 @@ where { // 1) Encode into an in-memory buffer let mut buf = Vec::new(); - bincode::serde::encode_into_std_write(data, &mut buf, BINCODE_CONFIG) - .map_err(|err| match err { + bincode::serde::encode_into_std_write(data, &mut buf, BINCODE_CONFIG).map_err( + |err| match err { // Ran out of room while encoding - EncodeError::UnexpectedEnd => - SnapshotError::Io(libc::EIO), + EncodeError::UnexpectedEnd => SnapshotError::Io(libc::EIO), // Underlying IO failure during encode (index tells how many bytes got written) - EncodeError::Io { inner, .. } => - SnapshotError::Io(inner.raw_os_error().unwrap_or(libc::EIO)), + EncodeError::Io { inner, .. } => { + SnapshotError::Io(inner.raw_os_error().unwrap_or(libc::EIO)) + } // Any other encode error we surface as Serde - other => - SnapshotError::Serde(other.to_string()), - })?; + other => SnapshotError::Serde(other.to_string()), + }, + )?; // 2) Flush that buffer to the target writer writer @@ -270,9 +279,7 @@ mod tests { let snapshot = SnapshotHdr::new(Version::new(1, 6, 1)); assert!(matches!( - Snapshot::load::<_, u8>( - &mut snapshot_data.as_slice(), - ), + Snapshot::load::<_, u8>(&mut snapshot_data.as_slice(),), Err(SnapshotError::InvalidSnapshotSize) )); } @@ -337,4 +344,62 @@ mod tests { Err(SnapshotError::Crc64(_)) )); } + + #[test] + fn test_bad_version() { + let mut data = vec![0u8; 100]; + + // We write a snapshot with version "v1.3.12" + let snapshot = Snapshot::new(Version::new(1, 3, 12)); + snapshot.save(&mut data.as_mut_slice(), &42u8).unwrap(); + + // Different major versions should not work + assert!(matches!( + Snapshot::load_with_version_check::<_, u8>( + &mut data.as_slice(), + Version::new(2, 3, 12) + ), + Err(SnapshotError::InvalidFormatVersion(Version { + major: 1, + minor: 3, + patch: 12, + .. + })) + )); + assert!(matches!( + snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), Version::new(0, 3, 12)), + Err(SnapshotError::InvalidFormatVersion(Version { + major: 1, + minor: 3, + patch: 12, + .. + })) + )); + + // We can't support minor versions bigger than ours + assert!(matches!( + snapshot.load_with_version_check::<_, u8>(&mut data.as_slice(), Version::new(1, 2, 12)), + Err(SnapshotError::InvalidFormatVersion(Version { + major: 1, + minor: 3, + patch: 12, + .. + })) + )); + + // But we can support minor versions smaller or equeal to ours. We also support + // all patch versions within our supported major.minor version. + snapshot + .load_with_version_check::<_, u8>(&mut data.as_slice(), Version::new(1, 4, 12)) + .unwrap(); + snapshot + .load_with_version_check::<_, u8>(&mut data.as_slice(), Version::new(1, 3, 0)) + .unwrap(); + snapshot + .load_with_version_check::<_, u8>(&mut data.as_slice(), Version::new(1, 3, 12)) + .unwrap(); + snapshot + .load_with_version_check::<_, u8>(&mut data.as_slice(), Version::new(1, 3, 1024)) + .unwrap(); + } }