Skip to content

Commit

Permalink
fix(vfs) Opening in append-mode must ignore seek operations.
Browse files Browse the repository at this point in the history
When opening a file with the `append` option turned on, all `seek`
operations must be ignored. As described by
[`open(2)`](https://man7.org/linux/man-pages/man2/open.2.html), the
`O_APPEND` option describes this behavior well:

> Before each write(2), the file offset is positioned at
> the end of the file, as if with lseek(2).  The
> modification of the file offset and the write operation
> are performed as a single atomic step.
>
> O_APPEND may lead to corrupted files on NFS filesystems
> if more than one process appends data to a file at once.
> This is because NFS does not support appending to a file,
> so the client kernel has to simulate it, which can't be
> done without a race condition.

This patch implements that behavior.
Also, this patch rewind the file cursor if opened in read-mode.
  • Loading branch information
Hywan committed Aug 31, 2021
1 parent a1ecfe0 commit c1328af
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 9 deletions.
27 changes: 23 additions & 4 deletions lib/vfs/src/mem_fs/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub(super) struct FileHandle {
filesystem: FileSystem,
readable: bool,
writable: bool,
append_mode: bool,
}

impl FileHandle {
Expand All @@ -31,12 +32,14 @@ impl FileHandle {
filesystem: FileSystem,
readable: bool,
writable: bool,
append_mode: bool,
) -> Self {
Self {
inode,
filesystem,
readable,
writable,
append_mode,
}
}
}
Expand Down Expand Up @@ -504,6 +507,24 @@ impl Read for FileHandle {

impl Seek for FileHandle {
fn seek(&mut self, position: io::SeekFrom) -> io::Result<u64> {
// In `append` mode, it's not possible to seek in the file. In
// [`open(2)`](https://man7.org/linux/man-pages/man2/open.2.html),
// the `O_APPEND` option describes this behavior well:
//
// > Before each write(2), the file offset is positioned at
// > the end of the file, as if with lseek(2). The
// > modification of the file offset and the write operation
// > are performed as a single atomic step.
// >
// > O_APPEND may lead to corrupted files on NFS filesystems
// > if more than one process appends data to a file at once.
// > This is because NFS does not support appending to a file,
// > so the client kernel has to simulate it, which can't be
// > done without a race condition.
if self.append_mode {
return Ok(0);
}

let mut fs =
self.filesystem.inner.try_write().map_err(|_| {
io::Error::new(io::ErrorKind::Other, "failed to acquire a write lock")
Expand Down Expand Up @@ -929,7 +950,6 @@ impl Write for File {
// The cursor is at the end of the buffer: happy path!
position if position == self.buffer.len() => {
self.buffer.extend_from_slice(buf);
self.cursor += buf.len();
}

// The cursor is at the beginning of the buffer (and the
Expand All @@ -941,7 +961,6 @@ impl Write for File {
new_buffer.append(&mut self.buffer);

self.buffer = new_buffer;
self.cursor += buf.len();
}

// The cursor is somewhere in the buffer: not the happy path.
Expand All @@ -951,11 +970,11 @@ impl Write for File {
let mut remainder = self.buffer.split_off(position);
self.buffer.extend_from_slice(buf);
self.buffer.append(&mut remainder);

self.cursor += buf.len();
}
}

self.cursor += buf.len();

Ok(buf.len())
}

Expand Down
36 changes: 31 additions & 5 deletions lib/vfs/src/mem_fs/file_opener.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ impl crate::FileOpener for FileOpener {
if append {
file.seek(io::SeekFrom::End(0))?;
}
// Otherwise, move the cursor to the start.
else {
file.seek(io::SeekFrom::Start(0))?;
}
}

_ => return Err(FsError::NotAFile),
Expand Down Expand Up @@ -153,6 +157,7 @@ impl crate::FileOpener for FileOpener {
self.filesystem.clone(),
read,
write || append || truncate,
append,
)))
}
}
Expand Down Expand Up @@ -333,18 +338,39 @@ mod test_file_opener {

let mut file = fs
.new_open_options()
.write(true)
.append(true)
.open(path!("/foo.txt"))
.expect("failed to open `foo.txt`");

assert!(
matches!(file.seek(io::SeekFrom::Current(0)), Ok(6)),
"checking the current position is 6",
matches!(file.seek(io::SeekFrom::Current(0)), Ok(0)),
"checking the current position in append-mode is 0",
);
assert!(
matches!(file.seek(io::SeekFrom::End(0)), Ok(6)),
"checking the size is 6",
matches!(file.seek(io::SeekFrom::Start(0)), Ok(0)),
"trying to rewind in append-mode",
);
assert!(matches!(file.write(b"baz"), Ok(3)), "writing `baz`");

let mut file = fs
.new_open_options()
.read(true)
.open(path!("/foo.txt"))
.expect("failed to open `foo.txt");

assert!(
matches!(file.seek(io::SeekFrom::Current(0)), Ok(0)),
"checking the current position is read-mode is 0",
);

let mut string = String::new();
assert!(
matches!(file.read_to_string(&mut string), Ok(9)),
"reading the entire `foo.txt` file",
);
assert_eq!(
string, "foobarbaz",
"checking append-mode is ignoring seek operations",
);
}

Expand Down

0 comments on commit c1328af

Please sign in to comment.