Panic when comparing two databases

[Susurrus]

First off, love that this tool exists! I use syncthing and occassionally my databases get out of sync and I have to resolve them, but I'm uncertain what the difference is! However I ran into an issue when comparing my two databases:

$ RUST_BACKTRACE=1 cargo run -- file1.kdbx file2.kdbx --same-password
Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/keepass-diff /home/susurrus/Personal/OneRing.kdbx /home/susurrus/Personal/OneRing.sync-conflict-20210404-004755-OP4M5NG.kdbx --same-password`
Password for both files: 
thread 'main' panicked at 'range end index 1398351512 out of range for slice of length 237116', /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/hmac_block_stream.rs:21:22
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::slice::index::slice_end_index_len_fail
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/slice/index.rs:41:5
   3: <core::ops::range::Range<usize> as core::slice::index::SliceIndex<[T]>>::index
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:238:13
   4: core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:15:9
   5: keepass::hmac_block_stream::read_hmac_block_stream
             at /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/hmac_block_stream.rs:21:22
   6: keepass::parse::kdbx4::parse
             at /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/parse/kdbx4.rs:260:9
   7: keepass::db::Database::open
             at /home/susurrus/.cargo/registry/src/github.com-1ecc6299db9ec823/keepass-0.4.7/src/db.rs:262:17
   8: keepass_diff::kdbx_to_group::{{closure}}
             at ./src/main.rs:189:22
   9: core::result::Result<T,E>::and_then
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:708:22
  10: keepass_diff::kdbx_to_group
             at ./src/main.rs:186:5
  11: keepass_diff::main
             at ./src/main.rs:152:24
  12: core::ops::function::FnOnce::call_once
             at /home/susurrus/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

If I compare file1.kdbx with itself, I don't run into any issues. However if I compare file2.kdbx, I do run into the same error. So I guess it's an issue with the second database. However, I can successfully open it with KeePassXC 2.6.4, so I guess it's a library parsing issue.

As a user I wouldn't expect a panic message, but a nice error message would be useful: in this case probably one that would suggest I report a bug against keepass directly. However, it'd probably be good to have some debug script I could run that would provide a good error report to provide there. I'm not certain what to say in my report. However, since this is a panic in the underlying library (hmac_block_stream.rs:21) I don't know if there's a way to catch that. Let me know if you'd like me to report this upstream instead or how I can help here. I do know some Rust (was really excited that this tool was in it!) so happy to help dig into this!

[Narigo]

Thanks for the report @Susurrus ! Testing the file with itself is a great suggestion for future bug reports actually 🙂

Do you think wrapping the call to the underlying keepass library with std::panic::catch_unwind might help?

Posting an issue upstream would make sense anyways as it should fail and tell the error instead of failing with a panic.

[Susurrus]

Do you think wrapping the call to the underlying keepass library with std::panic::catch_unwind might help?

It might help in this situation, though it doesn't look like it should be used for this specific situation, where an underlying library may panic. It looks like it's more designed to prevent panics in Rust from causing issues in other languages that might be calling into it. I think the right answer here is to resolve this within keepass. Maybe it'd be sufficient to document in the README that if the backtrace looks like it's in the keepass library, to file a bug directly with them?

I'll file a bug with upstream and see what they say.

[Susurrus]

Opened sseemayer/keepass-rs#32

[Susurrus]

I just compared them and it didn't crash:

❯ cargo run -- ~/Downloads/test2.kdbx ~/Downloads/Test1.kdbx 
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s
     Running `target/debug/keepass-diff /home/susurrus/Downloads/test2.kdbx /home/susurrus/Downloads/Test1.kdbx`
Password for file /home/susurrus/Downloads/test2.kdbx:
Password for file /home/susurrus/Downloads/Test1.kdbx:
+ [Root, Test1]
- [Root, Test2]

On Sun, May 2, 2021, at 4:32 AM, Niels wrote:

I removed rust on my pc, so it's difficult to test again. But on mac I get a message that complains abut the format. Can you compare the both 2 files? test.zip https://github.com/Narigo/keepass-diff/files/6411217/test.zip The password is geheim.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Narigo/keepass-diff/issues/32#issuecomment-830794860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABSMG6S5IHVRPLONIKOTODTLUZ4XANCNFSM42NCY3CQ.

[Susurrus]

I just tried to reproduce this today using my own databased and everything worked fine. I wonder if this was an error introduced by KeepassDX that was fixed in an update and since I've opened and resaved the "corrupted" one, it was fixed automatically. Anyways, since I can't reproduce this, going ahead and closing it.