Skip to content
This repository has been archived by the owner on Jul 17, 2023. It is now read-only.

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

Merged
merged 3 commits into from
Jul 14, 2023

Conversation

00xc
Copy link
Contributor

@00xc 00xc commented Jun 22, 2023

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.
    if offset + data_len > config_len {
  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):
    let (_, right) = config_slice.split_at_mut(offset as usize);
    right.copy_from_slice(data);
    }

@00xc
Copy link
Contributor Author

00xc commented Jun 22, 2023

See also firecracker-microvm/firecracker#3869

@ZizhengBian
Copy link
Collaborator

Please fix ut/clippy

@studychao
Copy link
Member

@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 00xc force-pushed the dbs-virtio-devices/balloon branch from 6f24a1f to 24351b8 Compare July 13, 2023 16:30
@00xc
Copy link
Contributor Author

00xc commented Jul 13, 2023

Should be good now

1. Fix potential integer overflow when calculating the end of the
write destination (`offset + data_len`).

2. Fix a potential panic when copying into the destination slice due
to size mismatch.

Signed-off-by: Carlos López <[email protected]>
@00xc 00xc force-pushed the dbs-virtio-devices/balloon branch from f24796c to 793206a Compare July 13, 2023 21:41
@studychao
Copy link
Member

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

Tip: we are using Rust 1.70.

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

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

❗ 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%) ⬇️
dbs-miniball ?
dbs-upcall 94.31% <ø> (ø)
dbs-utils ?
dbs-virtio-devices 87.29% <66.66%> (-0.02%) ⬇️

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%) ⬇️

... and 14 files with indirect coverage changes

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

@00xc
Copy link
Contributor Author

00xc commented Jul 14, 2023

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

Signed-off-by: Carlos López <[email protected]>
@00xc 00xc force-pushed the dbs-virtio-devices/balloon branch from ca956ff to da70713 Compare July 14, 2023 09:04
The new device is public but behind a private module. Add a `pub use`
for the exports of the module.

Signed-off-by: Carlos López <[email protected]>
@studychao
Copy link
Member

@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
Copy link
Member

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

@00xc
Copy link
Contributor Author

00xc commented Jul 14, 2023

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

Thanks!

@studychao studychao merged commit 0671f18 into openanolis:main Jul 14, 2023
6 of 9 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants