Skip to content

fix(vfs): 修复 pread64 系统调用的兼容性和错误处理 #1398

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.

Sign up for GitHub

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

Merged
fslongjin merged 12 commits into from
Dec 1, 2025
Merged

fix(vfs): 修复 pread64 系统调用的兼容性和错误处理 #1398

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
66b967e
fs: 修复 pread64 系统调用的兼容性和错误处理
kaleidoscope416 Nov 21, 2025
696122e
test
kaleidoscope416 Nov 21, 2025
c8c4dd9
copilot
kaleidoscope416 Nov 22, 2025
c716be8
fmt
kaleidoscope416 Nov 24, 2025
3f3ef9f
删去冗余的负数判断
kaleidoscope416 Nov 24, 2025
a756668
增加注释
kaleidoscope416 Nov 24, 2025
94d0a91
1
kaleidoscope416 Nov 26, 2025
0dc99c0
Merge remote-tracking branch 'origin/master' into jiangruizhi-fix/sys…
fslongjin Nov 26, 2025
667c09c
refactor(syscall): improve user buffer access checks in pread64 and u…
fslongjin Nov 26, 2025
cbad321
禁用preadv的不稳定测试
kaleidoscope416 Nov 29, 2025
030dfbe
Merge remote-tracking branch 'origin/master' into jiangruizhi-fix/sys…
fslongjin Dec 1, 2025
8761ffe
fix(vfs): 修复文件可读性检查中的模式比较错误
fslongjin Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions kernel/src/filesystem/vfs/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,9 +407,16 @@ impl File {
/// @brief 判断当前文件是否可读
#[inline]
pub fn readable(&self) -> Result<(), SystemError> {
let mode = *self.mode.read();

// 检查是否是O_PATH文件描述符
if mode.contains(FileMode::O_PATH) {
return Err(SystemError::EBADF);
}

// 暂时认为只要不是write only, 就可读
if *self.mode.read() == FileMode::O_WRONLY {
return Err(SystemError::EPERM);
if mode.accmode() == FileMode::O_WRONLY.bits() {
return Err(SystemError::EBADF);
}

return Ok(());
Expand Down
20 changes: 19 additions & 1 deletion kernel/src/filesystem/vfs/syscall/sys_pread64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use system_error::SystemError;

use crate::arch::interrupt::TrapFrame;
use crate::arch::syscall::nr::SYS_PREAD64;
use crate::filesystem::vfs::FileType;
use crate::process::ProcessManager;
use crate::syscall::table::FormattedSyscallParam;
use crate::syscall::table::Syscall;
Expand Down Expand Up @@ -35,7 +36,18 @@ impl Syscall for SysPread64Handle {
let len = Self::len(args);
let offset = Self::offset(args);

let mut user_buffer_writer = UserBufferWriter::new(buf_vaddr, len, frame.is_from_user())?;
// 检查offset是否为负数
if (offset as isize) < 0 {
return Err(SystemError::EINVAL);
}

Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The check (offset as isize) < 0 is redundant with the check on line 47 offset > i64::MAX as usize. Both conditions check if offset >= 2^63 (on 64-bit systems). The first check interprets the value as signed and checks if the sign bit is set, while the second explicitly compares against i64::MAX. Consider removing the first check to avoid duplication and improve code clarity.

Suggested change
// 检查offset是否为负数
if (offset as isize) < 0 {
return Err(SystemError::EINVAL);
}

Copilot uses AI. Check for mistakes.
// 检查offset + len是否溢出
if offset.checked_add(len).is_none() || offset > i64::MAX as usize {
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Incomplete overflow check: The validation should also check if offset + len exceeds i64::MAX, not just offset alone. If offset is valid but offset + len > i64::MAX, this should also return EINVAL.

Consider: if offset.checked_add(len).map_or(true, |sum| sum > i64::MAX as usize) || offset > i64::MAX as usize

Or split into clearer logic:

let end_pos = offset.checked_add(len).ok_or(SystemError::EINVAL)?;
if offset > i64::MAX as usize || end_pos > i64::MAX as usize {
    return Err(SystemError::EINVAL);
}
Suggested change
// 检查offset + len是否溢出
if offset.checked_add(len).is_none() || offset > i64::MAX as usize {
// 检查offset + len是否溢出，并且两者都不能超过i64::MAX
let end_pos = offset.checked_add(len).ok_or(SystemError::EINVAL)?;
if offset > i64::MAX as usize || end_pos > i64::MAX as usize {

Copilot uses AI. Check for mistakes.
return Err(SystemError::EINVAL);
}

let mut user_buffer_writer =
UserBufferWriter::new_checked(buf_vaddr, len, frame.is_from_user())?;
Copy link
Member

@fslongjin fslongjin Nov 25, 2025

Choose a reason for hiding this comment

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

要求用户缓冲区已经映射应该是不对的。可能是pagefault延迟映射。应该是使用protected的那个，校验vma

let user_buf = user_buffer_writer.buffer(0)?;

let binding = ProcessManager::current_pcb().fd_table();
Expand All @@ -48,6 +60,12 @@ impl Syscall for SysPread64Handle {
// Drop guard to avoid scheduling issues
drop(fd_table_guard);

// 检查是否是管道/Socket (ESPIPE)
let md = file.metadata()?;
if md.file_type == FileType::Pipe || md.file_type == FileType::Socket {
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Incomplete file type check for non-seekable files: The check should also include FileType::CharDevice to be consistent with lseek() at file.rs:364, which returns ESPIPE for both Pipe and CharDevice. Character devices like terminals are non-seekable and should fail pread64 with ESPIPE.

Should be: if md.file_type == FileType::Pipe || md.file_type == FileType::Socket || md.file_type == FileType::CharDevice

Suggested change
if md.file_type == FileType::Pipe || md.file_type == FileType::Socket {
if md.file_type == FileType::Pipe || md.file_type == FileType::Socket || md.file_type == FileType::CharDevice {

Copilot uses AI. Check for mistakes.
return Err(SystemError::ESPIPE);
}
Copy link

@cursor cursor bot Dec 1, 2025

Choose a reason for hiding this comment

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

Bug: CharDevice incorrectly included in ESPIPE file type check

The comment states this check is for pipes and sockets, but the code also includes FileType::CharDevice. According to POSIX, pread64 returns ESPIPE specifically for pipes, FIFOs, and sockets - not character devices. Many character devices like /dev/null and /dev/zero (which are FileType::CharDevice in this codebase) can legitimately support pread operations. Including CharDevice in this check will incorrectly return ESPIPE for valid pread64 calls on such devices.

Fix in Cursor Fix in Web


return file.pread(offset, len, user_buf);
}

Expand Down
Loading