Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: file read - offset and length checks #2092

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rinor
Copy link
Contributor

@rinor rinor commented Feb 24, 2025

resolve #2091

@rinor rinor marked this pull request as draft February 24, 2025 12:38
@francescolavra
Copy link
Member

There is a bug that affects both reads and writes when the offset is greater than the file length; at least for the read part, it is caused by

len = file_len - r.start;
, and I think it should be fixed by changing the file_io_complete() function so that it avoids adjusting the return value when 0 bytes have been read, e.g. by changing the preceding if () condition to something like if ((len > 0) && (r.start + len > file_len)). I'm not sure that would fix the write issue too, though.

@rinor
Copy link
Contributor Author

rinor commented Feb 24, 2025

well writes are a bit weird and would argue that nanos is more posix compliant than linux (bug) when it comes to pwrite64.

From https://www.man7.org/linux/man-pages/man2/pwrite.2.html :
Bugs
POSIX requires that opening a file with the O_APPEND flag should have no effect on the location at which pwrite() writes data. However, on Linux, if a file is opened with O_APPEND, pwrite() appends data to the end of the file, regardless of the value of offset.

that means that right now this works the same on both nanos/linux alowing you to seek and fill the "gaps" with 0x00

file, err := os.OpenFile(fileName, os.O_RDWR|os.O_CREATE, 0666)
offset := fileInfo.Size() + 1 // put offset outside file length
n, err = syscall.Pwrite(int(file.Fd()), []byte("12345"), offset)

but this one behaves different, on nanos behaves like the POSIX expects, but on Linux offset is not taken in consideration and data is just appended to the end of file without "gaps"

file, err := os.OpenFile(fileName, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
offset := fileInfo.Size() + 1 // put offset outside file length
n, err = syscall.Pwrite(int(file.Fd()), []byte("12345"), offset)

Is this what you are referring to about writes?

There is a bug that affects both reads and writes when the offset is greater than the file length;

@rinor rinor force-pushed the fix/file_read_offset branch from 26b63af to 5e48a8d Compare February 24, 2025 20:01
@rinor rinor marked this pull request as ready for review February 24, 2025 22:02
Copy link
Member

@francescolavra francescolavra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, somehow I was under the impression that pwrite() was suffering from the same bug as pread(), but I checked better now and that's not the case, so pwrite() is fine as it is.
I have just 2 trivial comments on the changes for fixing pread().

- Update boundary check for file I/O completion logic
- Set length to zero if starting position exceeds file length
@rinor rinor force-pushed the fix/file_read_offset branch from 5e48a8d to c8b27e6 Compare February 25, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: syscall/file_read (also file_readv probably) wrong return code when offset > file length
2 participants