Use u32 to describe vsock related buffer sizes #4637
Conversation
Move to u32 for vsock module. We can upsize from u32 to usize as needed. Signed-off-by: brandonpike <pikebrandon@att.net>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4637 +/- ##
=======================================
Coverage 82.08% 82.08%
=======================================
Files 255 255
Lines 31257 31262 +5
=======================================
+ Hits 25656 25661 +5
Misses 5601 5601
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if offset < self.len() as usize { | ||
| let expected = buf.len(); | ||
| if offset < self.len() { | ||
| let expected = u32::try_from(buf.len()).unwrap(); |
There was a problem hiding this comment.
Here, we can translate the error from try_from into some VolatileMemoryError to indicate that its impossible to read more than u32::MAX bytes from a descriptor chain
| let iov_len = u32::try_from(iov.iov_len).unwrap(); | ||
| if offset >= iov_len { | ||
| offset -= iov_len; |
There was a problem hiding this comment.
Mh, I think here its better to have a let mut offset = offset as usize in line 142, so that we do not need any casts around this arithmetic and the VolatileSlice constructor
There was a problem hiding this comment.
I agree, however, let's do let mut offset = u32::try_into(offset).unwrap. We should also add a comment on why this is ok. Something like:
`iov.iov_len` is a `usize` but it gets assigned from `DescriptorChain::len` which is a `u32`, so the guest cannot pass to us something that is bigger than `u32`. As a result
| offset = 0; | ||
|
|
||
| if slice.len() > len { | ||
| slice = slice.subslice(0, len)?; | ||
| if u32::try_from(slice.len()).unwrap() > len { |
There was a problem hiding this comment.
| if u32::try_from(slice.len()).unwrap() > len { | |
| if slice.len() > len as usize { |
Or also just a let len = len as usize; at the top of the method :). We mostly care that the API of these functions expresses the right thing, but casting up to usize inside of them is fine, since from there, the usize won't "escape" again.
| if offset < self.len() as usize { | ||
| let expected = buf.len(); | ||
| if offset < self.len() { | ||
| let expected = u32::try_from(buf.len()).unwrap(); |
| let mut total_bytes_read = 0; | ||
|
|
||
| for iov in &self.vecs { | ||
| if len == 0 { | ||
| break; | ||
| } | ||
|
|
||
| if offset >= iov.iov_len { | ||
| offset -= iov.iov_len; | ||
| let iov_len = u32::try_from(iov.iov_len).unwrap(); |
| offset = 0; | ||
|
|
||
| if slice.len() > len { | ||
| slice = slice.subslice(0, len)?; | ||
| if u32::try_from(slice.len()).unwrap() > len { |
| expected: expected as usize, | ||
| completed: bytes_read as usize, |
There was a problem hiding this comment.
Let's use expected.into() instead of as here.
There was a problem hiding this comment.
and everywhere else in this commit. We should always be able to use .into() or .try_into().unwrap() where appropriate.
| let iov_len = u32::try_from(iov.iov_len).unwrap(); | ||
| if offset >= iov_len { | ||
| offset -= iov_len; |
There was a problem hiding this comment.
I agree, however, let's do let mut offset = u32::try_into(offset).unwrap. We should also add a comment on why this is ok. Something like:
`iov.iov_len` is a `usize` but it gets assigned from `DescriptorChain::len` which is a `u32`, so the guest cannot pass to us something that is bigger than `u32`. As a result
| offset = 0; | ||
|
|
||
| if slice.len() > len { | ||
| slice = slice.subslice(0, len)?; | ||
| if u32::try_from(slice.len()).unwrap() > len { |
roypat
added
the
Status: Awaiting review
Move to u32 for vsock module. We can translate from u32 to usize as needed.
Changes
Closes #4627
Reason
Follow-up to #4556.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.