Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

thread 'main' panicked at 'attempt to add with overflow' #1

Open
tasleson opened this issue Apr 25, 2023 · 13 comments
Open

thread 'main' panicked at 'attempt to add with overflow' #1

tasleson opened this issue Apr 25, 2023 · 13 comments

Comments

@tasleson
Copy link
Contributor

When experimenting with blk-archive using a debug build I can encounter panics in the unpack and dump-stream commands. Running a release build runs without error as there is no integer overflow checks, it wraps. If this is the desired/correct behavior, I can submit a PR utilizing wrapping_add.

# ./blk-archive unpack -a /home/tony/archive_test/ -s 3963aaa87a699f7c --create /tmp/block_file
[                                        ] 0s remaining
thread 'main' panicked at 'attempt to add with overflow', src/stream.rs:824:21
stack backtrace:
   0:     0x559c27da659a - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hba918fcfc62e7e63
   1:     0x559c27dc7aee - core::fmt::write::hd0415f58b73c1765
   2:     0x559c27da3745 - std::io::Write::write_fmt::h348b97a955333f0c
   3:     0x559c27da6365 - std::sys_common::backtrace::print::h44cc2511b9123d29
   4:     0x559c27da7b1f - std::panicking::default_hook::{{closure}}::hc55200cc71caa2cb
   5:     0x559c27da785b - std::panicking::default_hook::h9ccd1febb2aa93b7
   6:     0x559c27da80c8 - std::panicking::rust_panic_with_hook::h3dfc181ac7662e7d
   7:     0x559c27da7f82 - std::panicking::begin_panic_handler::{{closure}}::h184a878f62ee69a8
   8:     0x559c27da6a06 - std::sys_common::backtrace::__rust_end_short_backtrace::h7c589a21be2c481f
   9:     0x559c27da7cd2 - rust_begin_unwind
  10:     0x559c27761073 - core::panicking::panic_fmt::h9b6860b1b6656890
  11:     0x559c2776110d - core::panicking::panic::h06f71e6020d16ba7
  12:     0x559c27776a78 - blk_archive::stream::MappingUnpacker::unpack::he4659a530cfebaa1
                               at /home/tony/blk-archive/src/stream.rs:824:21
  13:     0x559c278894ae - blk_archive::unpack::Unpacker<D>::unpack::h7970b507c9b3a4b4
                               at /home/tony/blk-archive/src/unpack.rs:164:41
  14:     0x559c277dd9bb - blk_archive::unpack::run_unpack::hbccbcd732e14df99
                               at /home/tony/blk-archive/src/unpack.rs:464:9
  15:     0x559c277638c0 - blk_archive::main_::h84c64dcf2cb6dddc
                               at /home/tony/blk-archive/src/main.rs:186:13
  16:     0x559c27763cd9 - blk_archive::main::h61f56ba734a4b328
                               at /home/tony/blk-archive/src/main.rs:204:22
  17:     0x559c27765a4b - core::ops::function::FnOnce::call_once::h7ddf96b2638b1e65
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/core/src/ops/function.rs:250:5
  18:     0x559c277640fe - std::sys_common::backtrace::__rust_begin_short_backtrace::hcf9c069e4691210f
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/std/src/sys_common/backtrace.rs:134:18
  19:     0x559c27765b11 - std::rt::lang_start::{{closure}}::h264747878cd00932
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/std/src/rt.rs:166:18
  20:     0x559c27d9e0fc - std::rt::lang_start_internal::ha1737cf821a325bc
  21:     0x559c27765aea - std::rt::lang_start::h99b77cb6969698c2
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/std/src/rt.rs:165:17
  22:     0x559c27763dee - main
  23:     0x7eff46d6f510 - __libc_start_call_main
  24:     0x7eff46d6f5c9 - __libc_start_main@GLIBC_2.2.5
  25:     0x559c27761705 - _start

blk-archive/src/stream.rs

Lines 823 to 825 in 58a24a2

OffsetDelta12 { delta } => {
self.vm_state.top().offset += delta as u32;
} 

# ./blk-archive dump-stream -a /home/tony/archive_test/ -s 3963aaa87a699f7c
0000000000       emit 9          0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000001        rot 14         0:0 0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000002      s.add 1          1:0 0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000003       emit 1          1:1 0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000004      unmap 196608     
0000000005       emit 1          1:2 0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000006      unmap 196608     
0000000007       fill 589824 (0) 
0000000008      unmap 7536640    
0000000009       emit 413        1:415 0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
000000000a      unmap 1769472    
000000000b       emit 302        1:717 0:9 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
thread 'main' panicked at 'attempt to add with overflow', src/stream.rs:1055:17
stack backtrace:
   0:     0x5625e587d59a - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hba918fcfc62e7e63
   1:     0x5625e589eaee - core::fmt::write::hd0415f58b73c1765
   2:     0x5625e587a745 - std::io::Write::write_fmt::h348b97a955333f0c
   3:     0x5625e587d365 - std::sys_common::backtrace::print::h44cc2511b9123d29
   4:     0x5625e587eb1f - std::panicking::default_hook::{{closure}}::hc55200cc71caa2cb
   5:     0x5625e587e85b - std::panicking::default_hook::h9ccd1febb2aa93b7
   6:     0x5625e587f0c8 - std::panicking::rust_panic_with_hook::h3dfc181ac7662e7d
   7:     0x5625e587ef82 - std::panicking::begin_panic_handler::{{closure}}::h184a878f62ee69a8
   8:     0x5625e587da06 - std::sys_common::backtrace::__rust_end_short_backtrace::h7c589a21be2c481f
   9:     0x5625e587ecd2 - rust_begin_unwind
  10:     0x5625e5238073 - core::panicking::panic_fmt::h9b6860b1b6656890
  11:     0x5625e523810d - core::panicking::panic::h06f71e6020d16ba7
  12:     0x5625e524f420 - blk_archive::stream::Dumper::exec::h09aedfd7f23d5c01
                               at /home/tony/blk-archive/src/stream.rs:1055:17
  13:     0x5625e5251f9d - blk_archive::stream::Dumper::dump::hba451343ea038a31
                               at /home/tony/blk-archive/src/stream.rs:1209:17
  14:     0x5625e529d782 - blk_archive::dump_stream::run::h785ef7b8b104be52
                               at /home/tony/blk-archive/src/dump_stream.rs:19:5
  15:     0x5625e523ab3c - blk_archive::main_::h84c64dcf2cb6dddc
                               at /home/tony/blk-archive/src/main.rs:195:13
  16:     0x5625e523acd9 - blk_archive::main::h61f56ba734a4b328
                               at /home/tony/blk-archive/src/main.rs:204:22
  17:     0x5625e523ca4b - core::ops::function::FnOnce::call_once::h7ddf96b2638b1e65
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/core/src/ops/function.rs:250:5
  18:     0x5625e523b0fe - std::sys_common::backtrace::__rust_begin_short_backtrace::hcf9c069e4691210f
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/std/src/sys_common/backtrace.rs:134:18
  19:     0x5625e523cb11 - std::rt::lang_start::{{closure}}::h264747878cd00932
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/std/src/rt.rs:166:18
  20:     0x5625e58750fc - std::rt::lang_start_internal::ha1737cf821a325bc
  21:     0x5625e523caea - std::rt::lang_start::h99b77cb6969698c2
                               at /builddir/build/BUILD/rustc-1.69.0-src/library/std/src/rt.rs:165:17
  22:     0x5625e523adee - main
  23:     0x7f7d4a206510 - __libc_start_call_main
  24:     0x7f7d4a2065c9 - __libc_start_main@GLIBC_2.2.5
  25:     0x5625e5238705 - _start
  26:                0x0 - <unknown>

blk-archive/src/stream.rs

Lines 1053 to 1056 in 58a24a2

OffsetDelta12 { delta } => {
self.stats.offset_delta12 += 1;
self.vm_state.top().offset += *delta as u32;
}

@jthornber
Copy link
Owner

jthornber commented Apr 26, 2023 via email

@jthornber
Copy link
Owner

jthornber commented Apr 26, 2023 via email

@tasleson
Copy link
Contributor Author

Does it work in a release build?

Yes, it appears to work. Basic testing shows that the unpacked data matches source.

@tasleson
Copy link
Contributor Author

For my own understanding of what's happening I added the following debug

diff --git a/src/stream.rs b/src/stream.rs
index c4c0707..21c51c9 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -821,7 +821,13 @@ impl MappingUnpacker {
                     self.vm_state.top().offset += delta as u32;
                 }
                 OffsetDelta12 { delta } => {
-                    self.vm_state.top().offset += delta as u32;
+                    let l_offset = self.vm_state.top().offset;
+                    let result = self.vm_state.top().offset.checked_add(delta as u32);
+
+                    match result {
+                        Some(x) => self.vm_state.top().offset = x,
+                        None => panic!("self.vm_state.top().offset = {}, i16 delta = {} u32 delta = {} wrapping add = {}", l_offset, delta, delta as u32, l_offset.wrapping_add(delta as u32)),
+                    }
                 }
                 Emit4 { len } => {
                     self.emit_run(&mut entries, len as usize);

and got this:

thread 'main' panicked at 'self.vm_state.top().offset = 717, i16 delta = -413 u32 delta = 4294966883 wrapping add = 304', src/stream.rs:829:33

Does this match your expectations?

@jthornber
Copy link
Owner

jthornber commented Apr 26, 2023 via email

@jthornber
Copy link
Owner

jthornber commented Apr 27, 2023 via email

@tasleson
Copy link
Contributor Author

Hi Tony, I just pushed a couple of changes. I haven't been able to reproduce your original issue where next_slab failed though.

Hi Joe,

With these latest changes I'm able to pack/unpack without the "attempt to add with overflow". However, you missed the spots in the dump-stream path to do the wrapping add, eg.

# target/debug/blk-archive dump-stream -a /home/tony/archive0/ -s c06a035498b85641
0000000000      emit 5           0:5 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000001      next             1:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000002      emit 1           1:1 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000003      unmap 196608     
0000000004      emit 1           1:2 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000005      unmap 196608     
0000000006      fill 589824 (0)  
0000000007      unmap 7536640    
0000000008      emit 259         1:261 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 0:0 
0000000009      unmap 1769472    
thread 'main' panicked at 'attempt to add with overflow', src/stream.rs:1081:17
stack backtrace:
   0:     0x55d3dcb0975a - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hba918fcfc62e7e63
   1:     0x55d3dcb2acae - core::fmt::write::hd0415f58b73c1765
   2:     0x55d3dcb06905 - std::io::Write::write_fmt::h348b97a955333f0c
   3:     0x55d3dcb09525 - std::sys_common::backtrace::print::h44cc2511b9123d29
   4:     0x55d3dcb0acdf - std::panicking::default_hook::{{closure}}::hc55200cc71caa2cb
   5:     0x55d3dcb0aa1b - std::panicking::default_hook::h9ccd1febb2aa93b7
   6:     0x55d3dcb0b288 - std::panicking::rust_panic_with_hook::h3dfc181ac7662e7d
   7:     0x55d3dcb0b142 - std::panicking::begin_panic_handler::{{closure}}::h184a878f62ee69a8
   8:     0x55d3dcb09bc6 - std::sys_common::backtrace::__rust_end_short_backtrace::h7c589a21be2c481f
   9:     0x55d3dcb0ae92 - rust_begin_unwind
  10:     0x55d3dc4b9073 - core::panicking::panic_fmt::h9b6860b1b6656890
  11:     0x55d3dc4b910d - core::panicking::panic::h06f71e6020d16ba7
  12:     0x55d3dc4d0391 - blk_archive::stream::Dumper::exec::h04cd02941cb8835c
                               at /home/tony/blk-archive/src/stream.rs:1081:17
...

@tasleson
Copy link
Contributor Author

So these changes to the stream instruction set are incompatible. I see Add fields to slab file header to describe it's contents and format version in the TODO which would address this? I'm guessing that at some point the tool would maintain backwards compatibility when the archive format/instruction stream changes, or at the very least exit early with a message that the tool is incompatible with archive version.

@jthornber
Copy link
Owner

jthornber commented Apr 27, 2023 via email

@tasleson
Copy link
Contributor Author

Yes, we haven't got to the point where the file formats are stable enough to start versioning and doing backwards compatibility. Lot's of items in the 'beta' section of the TODO will require new fields. In particular the damage resilience. Are you interested in tackling any of this?

I'm here to help, so sure.

Also I'm Just experimenting some more 😸 but this isn't working as well or maybe I'm doing something wrong...

# target/debug/blk-archive unpack -a /home/tony/archive0/ -s c06a035498b85641 --create /tmp/verify
# target/debug/blk-archive verify -a /home/tony/archive0/ -s c06a035498b85641 /tmp/verify 
verify failed at offset ~8192000: expected unmapped, got data

I'm thinking some automated CI would be useful. I could setup CircleCI in my blk-archive fork to demo if you're interested? They provide a fairly generous amount of monthly resources for open source projects.

@jthornber
Copy link
Owner

jthornber commented Apr 27, 2023 via email

@tasleson
Copy link
Contributor Author

I suspect you created the stream using a thin device, which contained unmaps. Then extracted to a file (which can't contain unmaps so fills with zeroes). So we need to update verify to handle this case. This is exactly the sort of thing we want to to find by using the tools more.

The documentation mentions the ability for "thick" and "thin" devices, but I've not been able to pack anything other than a thinly provisioned block device. Thus in all my testing, the source block device is indeed thin. I haven't tried packing individual files into an archive.

@tasleson
Copy link
Contributor Author

tasleson commented May 4, 2023

I suspect you created the stream using a thin device, which contained unmaps. Then extracted to a file (which can't contain unmaps so fills with zeroes). So we need to update verify to handle this case. This is exactly the sort of thing we want to to find by using the tools more.

Actually, I think the issue is I ran verify against the unpacked block device instead of the source block device. The tool states verifies stream in the archive against the original file/dev I don't think there is a way for blk-archive to check for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jthornber @tasleson