mdsteele / rust-cfb

Rust library for reading/writing Compound File Binary (structured storage) files
MIT License
46 stars 20 forks source link

Incorrect MiniFAT chain length (header says 1, actual is 2) #12

Closed surban closed 2 years ago

surban commented 2 years ago

I am getting the above error when reopening a file that was created with this crate. However, commenting out the appropriate check in lib.rs:403 makes it readable.

I can provide you with the file, but I would rather not upload it publicly.

mdsteele commented 2 years ago

Sure, if you want you can email me the file (mdsteele@alum.mit.edu), and I can try to figure out what's going on.

surban commented 2 years ago

I've mailed you the file.

Also, I've run a few more checks. It seems that I can read all entries and their contents within the CFB file just fine when I comment out the check for the MiniFAT chain length. So it seems that the file is indeed intact, just somewhere the header isn't updated when the MiniFAT grows.

mdsteele commented 2 years ago

Thanks, I've received the file, and confirmed that I get the same error when I try to read it with the cfb crate (and that it seems to work fine otherwise when I comment out the check in lib.rs:403). It looks like the MiniFAT in that file has just three populated entries, and then a whole bunch of extra free entries—enough that the MiniFAT chain takes up two sectors—but the CFB file header incorrectly lists the number of MiniFAT sectors as 1.

Possibly cfb should be less strict about opening files with incorrect header values, but if it's generating incorrect header values, I'd like to track that down.

I'm trying to write an integration test to see if I can get the cfb crate to create a similar sort of CFB file (then save and reopen it) so I can reproduce the problem in a test, but (unfortunately?) the test I wrote passes just fine and the cfb crate seems to correctly update the header with the number of MiniFAT sectors. Presumably my test is constructing its CFB file in a different way than you did, such that it doesn't tickle the bug? If your code that used cfb to generate the file you sent me happens to be visible somewhere public, I could take a look.

Alternatively, if you were using an older version of cfb, there's always the chance that my test passes because this bug somehow got already fixed; do you know what version of cfb you used to create that file?

surban commented 2 years ago

Hi,

thanks for investigating. I've managed to produce a somewhat reliable test case at https://github.com/surban/rust-cfb/tree/test2.

$ cargo test --release large_file

...

997: "1097105465207881823/535068735256626241/1654730976230085652/1479878657273552817/16594774604723100142/16282592443348169539/5141153473456206402/7474220944655359257/18386555722536719420/3640068409734246024/file"
998: "15233366004687614841/17110961905071956288/3461799207949843587/2956477538331761186/14580975933060640102/10096965140567487440/10857002502456766753/1995644454631255475/8816817431223778358/11687919955186910049/file"
999: "15325735911042825732/11527156048012980787/11262365494089736006/13231614731720573998/14408075340521169662/10109310091362561475/15730777940568448675/11054144027193995632/17006024488028320019/13898819080185493565/file"
thread 'large_file' panicked at 'open: Custom { kind: InvalidData, error: "Incorrect MiniFAT chain length (header says 1, actual is 2)" }', tests/large.rs:43:44
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    large_file

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.63s

error: test failed, to rerun pass '--test large'
mdsteele commented 2 years ago

Perfect, thanks for that test. I managed to shrink it down a bit such that it can reliably reproduce the error without taking too long to run in debug mode. Now I'm working on a fix.

mdsteele commented 2 years ago

I've pushed a change that I think should fix the problem of cfb generating files with incorrect num_minifat_sectors values in the header (along with a new regression test). Can you give it a try and see if it fixes your use case?

surban commented 2 years ago

Thanks for the fix.

Seems good so far!