openanolis / dragonball-sandbox

Dragonball-sandbox is a collection of Rust crates to help build custom Virtual Machine Monitors and hypervisors.
Apache License 2.0
88 stars 41 forks source link

dbs-virtio-devices: fix Balloon::write_config() bugs #290

Closed 00xc closed 1 year ago

00xc commented 1 year ago

Fix two bugs in Balloon::write_config():

  1. offset + data_len might overflow. This causes a panic in debug builds, and wraps around in release builds. https://github.com/openanolis/dragonball-sandbox/blob/8ed5e3fea96bf16747945d65c2b386542b28e1b7/crates/dbs-virtio-devices/src/balloon.rs#L634
  2. Source (data) and destination (right) slices' sizes might be mismatched in right.copy_from_slice(data), which causes a panic (this is documented). This can happen if data is shorter than the right side of config_slice.split_at_mut(offset): https://github.com/openanolis/dragonball-sandbox/blob/8ed5e3fea96bf16747945d65c2b386542b28e1b7/crates/dbs-virtio-devices/src/balloon.rs#L638-L640
00xc commented 1 year ago

See also firecracker-microvm/firecracker#3869

ZizhengBian commented 1 year ago

Please fix ut/clippy

studychao commented 1 year ago

@00xc Hi, could you please help fix the UT before this week since we are going to migrate all dbs crates to kata containers next week.

00xc commented 1 year ago

Should be good now

studychao commented 1 year ago

@00xc There seems several clippy warnings please help fix it.

Tip: we are using Rust 1.70.

codecov[bot] commented 1 year ago

Codecov Report

Merging #290 (793206a) into main (7cb8457) will decrease coverage by 0.37%. The diff coverage is 4.93%.

:exclamation: Current head 793206a differs from pull request most recent head 59a4ea6. Consider uploading reports for the commit 59a4ea6 to get more accurate results

@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
- Coverage   90.42%   90.05%   -0.37%     
==========================================
  Files          82       69      -13     
  Lines       24596    22047    -2549     
==========================================
- Hits        22240    19855    -2385     
+ Misses       2356     2192     -164     
Flag Coverage Δ
dbs-address-space 95.30% <ø> (ø)
dbs-allocator 94.98% <ø> (ø)
dbs-arch 96.37% <ø> (ø)
dbs-boot 94.90% <ø> (ø)
dbs-device 92.95% <ø> (ø)
dbs-interrupt ?
dbs-legacy-devices 73.01% <0.00%> (-19.77%) :arrow_down:
dbs-miniball ?
dbs-upcall 94.31% <ø> (ø)
dbs-utils ?
dbs-virtio-devices 87.29% <66.66%> (-0.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/dbs-legacy-devices/src/cmos.rs 0.00% <0.00%> (ø)
crates/dbs-legacy-devices/src/lib.rs 100.00% <ø> (ø)
crates/dbs-virtio-devices/src/balloon.rs 80.49% <66.66%> (-0.31%) :arrow_down:

... and 14 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

00xc commented 1 year ago

Those warnings popped up on code completely unrelated to this PR. I have fixed them, but that seems like a very unusual request.

studychao commented 1 year ago

@00xc , sorry, after dig into the reason for CI complaining for the code not in your PR, we found out that there is one PR merged without clippy pass. Do you mind fix that in this PR or I could raise a new PR to fix this ?

studychao commented 1 year ago

@00xc Hi, the PR itself looks good to me. I'll handle the clippy warnings.

00xc commented 1 year ago

@00xc Hi, the PR itself looks good to me. I'll handle the clippy warnings.

Thanks!