Conversation
There was a problem hiding this comment.
Pull request overview
Implements write support for the in-memory VFS (VfsCtxView), replacing the previous ReadOnly trap and adding a suite of unit tests validating POSIX-like behaviors around writing.
Changes:
- Implement
HostDescriptor::writefor regular files, including file extension, zero-fill gaps, and memory-limit enforcement via the limiter. - Add comprehensive unit tests for
writecovering success cases, offsets, EOF extension, directory errors, permission errors, and insufficient-memory behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| let node = Arc::clone(&desc.node); | ||
| let offset = offset as usize; |
There was a problem hiding this comment.
offset is a Filesize (WASI type) but is cast with offset as usize. On 32-bit hosts or when guests pass offsets > usize::MAX, this truncates and can write to the wrong location rather than returning an error. Please use a fallible conversion (usize::try_from(offset)) and trap with an appropriate error code when it doesn’t fit.
| let offset = offset as usize; | |
| let offset = usize::try_from(offset) | |
| .map_err(|_| FsError::trap(ErrorCode::Overflow))?; |
| match &mut guard.kind { | ||
| VfsNodeKind::File { content } => { | ||
| let current_len = content.len(); | ||
| let write_end = offset.saturating_add(buffer_len); |
There was a problem hiding this comment.
write_end = offset.saturating_add(buffer_len) hides arithmetic overflow. If offset + buffer_len overflows usize, this will silently clamp to usize::MAX, potentially attempting an enormous Vec::resize or producing incorrect accounting. Prefer checked_add and return an error when the requested end offset can’t be represented.
| let write_end = offset.saturating_add(buffer_len); | |
| let write_end = offset | |
| .checked_add(buffer_len) | |
| .ok_or_else(|| FsError::trap(ErrorCode::InsufficientMemory))?; |
| // Extend the file with zeros if writing past the current end | ||
| // Per POSIX: Writing past EOF extends the file | ||
| if offset > current_len { | ||
| content.resize(offset, 0); | ||
| } | ||
|
|
||
| // Ensure we have enough capacity for the write |
There was a problem hiding this comment.
The file extension logic does two resizes (resize(offset, 0) and later resize(write_end, 0)). Since write_end is already known, this can be simplified to a single resize to write_end (which also zero-fills any gap) to reduce reallocations and keep the control flow simpler.
| // Extend the file with zeros if writing past the current end | |
| // Per POSIX: Writing past EOF extends the file | |
| if offset > current_len { | |
| content.resize(offset, 0); | |
| } | |
| // Ensure we have enough capacity for the write | |
| // Extend the file with zeros if writing past the current end. | |
| // A single resize to write_end is sufficient: Vec::resize zero-fills | |
| // any newly allocated region, including any gap between the previous | |
| // end-of-file and the write offset, matching POSIX semantics. |
2a96695 to
1f00b30
Compare
Uh oh!
There was an error while loading. Please reload this page.