From 4ef929c165c62c52c10f2b882c9f8cc64d9902d6 Mon Sep 17 00:00:00 2001 From: Francis De Brabandere Date: Fri, 5 Apr 2024 23:52:31 +0200 Subject: [PATCH 1/4] fix: num_dir_sectors --- src/internal/directory.rs | 23 +++++++++++++++++++++++ src/lib.rs | 36 ++++++++++++++++++++++++++++++++++-- tests/malformed.rs | 28 ++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 2 deletions(-) diff --git a/src/internal/directory.rs b/src/internal/directory.rs index 8e22da9..a44f6ba 100644 --- a/src/internal/directory.rs +++ b/src/internal/directory.rs @@ -413,6 +413,7 @@ impl Directory { if self.dir_entries.len() % dir_entries_per_sector == 0 { let start_sector = self.dir_start_sector; self.allocator.extend_chain(start_sector, SectorInit::Dir)?; + self.update_num_dir_sectors()?; } // Add a new entry to the end of the directory and return it. let stream_id = self.dir_entries.len() as u32; @@ -420,6 +421,27 @@ impl Directory { Ok(stream_id) } + /// Increase header num_dir_sectors if version V4 + /// note: not updating this value breaks ole32 compatibility + fn update_num_dir_sectors(&mut self) -> io::Result<()> { + let start_sector = self.dir_start_sector; + if self.version() == Version::V4 { + let num_dir_sectors = self.count_directory_sectors(start_sector)?; + self.seek_within_header(40)?.write_u32::(num_dir_sectors)?; + } + Ok(()) + } + + fn count_directory_sectors(&mut self, start_sector: u32) -> io::Result { + let mut num_dir_sectors = 1; + let mut next_sector = self.allocator.next(start_sector)?; + while next_sector != consts::END_OF_CHAIN { + num_dir_sectors += 1; + next_sector = self.allocator.next(next_sector)?; + } + Ok(num_dir_sectors) + } + /// Deallocates the specified directory entry. fn free_dir_entry(&mut self, stream_id: u32) -> io::Result<()> { debug_assert_ne!(stream_id, consts::ROOT_STREAM_ID); @@ -428,6 +450,7 @@ impl Directory { *self.dir_entry_mut(stream_id) = dir_entry; // TODO: Truncate directory chain if last directory sector is now all // unallocated. + // In that case, also call update_num_dir_sectors() Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index 96c146f..468c4a9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -493,7 +493,15 @@ impl CompoundFile { let mut dir_entries = Vec::::new(); let mut seen_dir_sectors = FnvHashSet::default(); let mut current_dir_sector = header.first_dir_sector; + let mut dir_sector_count = 1; while current_dir_sector != consts::END_OF_CHAIN { + if validation.is_strict() && header.version == Version::V4 && dir_sector_count > header.num_dir_sectors { + invalid_data!( + "Directory chain includes at least {} sectors which is greater than header num_dir_sectors {}", + dir_sector_count, + header.num_dir_sectors + ); + } if current_dir_sector > consts::MAX_REGULAR_SECTOR { invalid_data!( "Directory chain includes invalid sector index {}", @@ -526,6 +534,7 @@ impl CompoundFile { } } current_dir_sector = allocator.next(current_dir_sector)?; + dir_sector_count += 1; } let mut directory = Directory::new( @@ -985,10 +994,11 @@ impl fmt::Debug for CompoundFile { #[cfg(test)] mod tests { - use std::io::{self, Cursor}; + use std::io::{self, Cursor, Read, Seek, SeekFrom, Write}; use std::mem::size_of; + use std::path::Path; - use byteorder::{LittleEndian, WriteBytesExt}; + use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; use crate::internal::{consts, DirEntry, Header, Version}; @@ -1048,6 +1058,28 @@ mod tests { // under Permissive validation. CompoundFile::open(Cursor::new(data)).expect("open"); } + + // Regression test for https://github.com/mdsteele/rust-cfb/issues/52. + #[test] + fn update_num_dir_sectors() { + // Create a CFB file with 2 sectors for the directory. + let cursor = Cursor::new(Vec::new()); + let mut comp = CompoundFile::create(cursor).unwrap(); + // root + 31 entries in the first sector + // 1 stream entry in the second sector + for i in 0..32 { + let path = format!("stream{}", i); + let path = Path::new(&path); + comp.create_stream(path).unwrap(); + } + comp.flush().unwrap(); + + // read num_dir_sectors from the header + let mut cursor = comp.into_inner(); + cursor.seek(SeekFrom::Start(40)).unwrap(); + let num_dir_sectors = cursor.read_u32::().unwrap(); + assert_eq!(num_dir_sectors, 2); + } } //===========================================================================// diff --git a/tests/malformed.rs b/tests/malformed.rs index 5971954..c0cc057 100644 --- a/tests/malformed.rs +++ b/tests/malformed.rs @@ -456,3 +456,31 @@ fn open_difat_terminate_freesect() { fn open_strict_difat_terminate_freesect() { CompoundFile::open_strict(difat_terminate_in_freesect()).unwrap(); } + +/// Regression test for https://github.com/mdsteele/rust-cfb/issues/52. +#[test] +#[should_panic( +expected = "Directory chain includes at least 2 sectors which is greater than header num_dir_sectors 1" +)] +fn invalid_num_dir_sectors_issue_52() { + // Create a CFB file with 2 sectors for the directory. + let cursor = Cursor::new(Vec::new()); + let mut comp = CompoundFile::create(cursor).unwrap(); + // root + 31 entries in the first sector + // 1 stream entry in the second sector + for i in 0..32 { + let path = format!("stream{}", i); + let path = Path::new(&path); + comp.create_stream(path).unwrap(); + } + comp.flush().unwrap(); + + // update the header and set num_dir_sectors = 1 instead of 2 + let mut cursor = comp.into_inner(); + cursor.seek(SeekFrom::Start(40)).unwrap(); + cursor.write_all(&[0x01, 0x00, 0x00, 0x00]).unwrap(); + cursor.flush().unwrap(); + + // Read the file back in. + CompoundFile::open_strict(cursor).unwrap(); +} From fcf3533a37637ccaf751edb1511489aa51d598aa Mon Sep 17 00:00:00 2001 From: Francis De Brabandere Date: Sat, 6 Apr 2024 00:29:53 +0200 Subject: [PATCH 2/4] improve test --- tests/malformed.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/malformed.rs b/tests/malformed.rs index c0cc057..045204b 100644 --- a/tests/malformed.rs +++ b/tests/malformed.rs @@ -478,7 +478,7 @@ fn invalid_num_dir_sectors_issue_52() { // update the header and set num_dir_sectors = 1 instead of 2 let mut cursor = comp.into_inner(); cursor.seek(SeekFrom::Start(40)).unwrap(); - cursor.write_all(&[0x01, 0x00, 0x00, 0x00]).unwrap(); + cursor.write_u32::(1).unwrap(); cursor.flush().unwrap(); // Read the file back in. From fa9298e37ddcf3db62683dcced3c55c86945376c Mon Sep 17 00:00:00 2001 From: Francis De Brabandere Date: Sun, 7 Apr 2024 20:18:14 +0200 Subject: [PATCH 3/4] fmt --- src/internal/directory.rs | 11 ++++++++--- src/lib.rs | 5 ++++- tests/malformed.rs | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/internal/directory.rs b/src/internal/directory.rs index a44f6ba..79535a0 100644 --- a/src/internal/directory.rs +++ b/src/internal/directory.rs @@ -426,13 +426,18 @@ impl Directory { fn update_num_dir_sectors(&mut self) -> io::Result<()> { let start_sector = self.dir_start_sector; if self.version() == Version::V4 { - let num_dir_sectors = self.count_directory_sectors(start_sector)?; - self.seek_within_header(40)?.write_u32::(num_dir_sectors)?; + let num_dir_sectors = + self.count_directory_sectors(start_sector)?; + self.seek_within_header(40)? + .write_u32::(num_dir_sectors)?; } Ok(()) } - fn count_directory_sectors(&mut self, start_sector: u32) -> io::Result { + fn count_directory_sectors( + &mut self, + start_sector: u32, + ) -> io::Result { let mut num_dir_sectors = 1; let mut next_sector = self.allocator.next(start_sector)?; while next_sector != consts::END_OF_CHAIN { diff --git a/src/lib.rs b/src/lib.rs index 468c4a9..6b406f3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -495,7 +495,10 @@ impl CompoundFile { let mut current_dir_sector = header.first_dir_sector; let mut dir_sector_count = 1; while current_dir_sector != consts::END_OF_CHAIN { - if validation.is_strict() && header.version == Version::V4 && dir_sector_count > header.num_dir_sectors { + if validation.is_strict() + && header.version == Version::V4 + && dir_sector_count > header.num_dir_sectors + { invalid_data!( "Directory chain includes at least {} sectors which is greater than header num_dir_sectors {}", dir_sector_count, diff --git a/tests/malformed.rs b/tests/malformed.rs index 045204b..273b6ea 100644 --- a/tests/malformed.rs +++ b/tests/malformed.rs @@ -460,7 +460,7 @@ fn open_strict_difat_terminate_freesect() { /// Regression test for https://github.com/mdsteele/rust-cfb/issues/52. #[test] #[should_panic( -expected = "Directory chain includes at least 2 sectors which is greater than header num_dir_sectors 1" + expected = "Directory chain includes at least 2 sectors which is greater than header num_dir_sectors 1" )] fn invalid_num_dir_sectors_issue_52() { // Create a CFB file with 2 sectors for the directory. From a6d2e00c7716cf6994ae119cc81101961c2afc5c Mon Sep 17 00:00:00 2001 From: Francis De Brabandere Date: Sun, 7 Apr 2024 20:48:03 +0200 Subject: [PATCH 4/4] remove unused imports --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 6b406f3..6cfe7d7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -997,7 +997,7 @@ impl fmt::Debug for CompoundFile { #[cfg(test)] mod tests { - use std::io::{self, Cursor, Read, Seek, SeekFrom, Write}; + use std::io::{self, Cursor, Seek, SeekFrom}; use std::mem::size_of; use std::path::Path;