Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion os/StarryOS/kernel/src/file/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ pub fn resolve_at(dirfd: c_int, path: Option<&str>, flags: u32) -> AxResult<Reso
let file_like = get_file_like(dirfd)?;
let f = file_like.clone();
Ok(if let Some(file) = f.downcast_ref::<File>() {
ResolveAtResult::File(file.inner().backend()?.location().clone())
// Use location() directly: backend() rejects PATH-only fds
// (BadFileDescriptor) which would break fstat(O_PATH-fd).
// man "O_PATH": fstat(2) is in the allowed-operations list.
// Fixes bug-open-path-fstat-ebadf.
ResolveAtResult::File(file.inner().location().clone())
} else if let Some(dir) = f.downcast_ref::<Directory>() {
ResolveAtResult::File(dir.inner().clone())
} else {
Expand Down
63 changes: 60 additions & 3 deletions os/StarryOS/kernel/src/syscall/fs/fd_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ fn flags_to_options(flags: c_int, mode: __kernel_mode_t, (uid, gid): (u32, u32))
if flags & O_CREAT != 0 {
options.create(true);
}
if flags & O_PATH != 0 {
options.path(true);
}
// O_EXCL only makes sense with O_CREAT (POSIX). Without O_CREAT, Linux
// ignores O_EXCL for existing files — busybox blkdiscard opens block
// devices with O_RDWR|O_EXCL (no O_CREAT).
Expand All @@ -59,6 +56,21 @@ fn flags_to_options(flags: c_int, mode: __kernel_mode_t, (uid, gid): (u32, u32))
if flags & O_DIRECT != 0 {
options.direct(true);
}
// O_PATH must be applied LAST and override conflicting flags: man 2 open
// "When O_PATH is specified in flags, flag bits other than O_CLOEXEC,
// O_DIRECTORY, and O_NOFOLLOW are ignored."
// Fixes bug-open-path-honors-excl / -path-creat-creates /
// -path-dir-write-eisdir / -path-sym-write-enoent.
if flags & O_PATH != 0 {
options.path(true);
// O_PATH: drop access mode (no I/O), drop create/excl/trunc/append
options.read(true).write(false);
options
.create(false)
.create_new(false)
.truncate(false)
.append(false);
}
options
}

Expand Down Expand Up @@ -140,6 +152,51 @@ pub fn sys_openat(
let path = vm_load_string(path)?;
debug!("sys_openat <= {dirfd} {path:?} {flags:#o} {mode:#o}");

let uflags = flags as u32;

// Pathname length check — man "ENAMETOOLONG — pathname was too long".
// starry path layer only checks per-component (255); add whole-path
// check (PATH_MAX = 4096). Fixes bug-open-pathmax-no-enametoolong.
if path.len() >= 4096 {
return Err(AxError::NameTooLong);
}

// Empty pathname → ENOENT. openat() does not accept AT_EMPTY_PATH.
// Fixes bug-openat-empty-path-no-enoent.
if path.is_empty() {
return Err(AxError::NotFound);
}

// O_CREAT|O_DIRECTORY is an invalid combination: open() cannot create
// a directory. man: "EINVAL — flags contains an invalid value."
// Fixes bug-open-creat-directory-einval.
// Exception: O_PATH ignores O_CREAT, so still allow PATH|CREAT|DIRECTORY.
if uflags & O_CREAT != 0 && uflags & O_DIRECTORY != 0 && uflags & O_PATH == 0 {
return Err(AxError::InvalidInput);
}

// O_TMPFILE requires O_RDWR or O_WRONLY. man: "EINVAL — O_TMPFILE was
// specified in flags, but neither O_WRONLY nor O_RDWR was specified."
// Fixes bug-open-tmpfile-no-einval.
//
// Exception: O_PATH ignores all flags except O_CLOEXEC/O_DIRECTORY/O_NOFOLLOW

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 小建议(非阻塞)O_PATH last-override 块中 options.read(true).write(false)read(true) 是为了 F_GETFL 正确返回访问模式位。建议加一行注释说明 read(true) 的目的(当前注释说 'drop access mode (no I/O)' 但 read(true) 实际上是设置访问模式位)。不过不影响正确性。

// (see flags_to_options PATH override below). On Linux, O_PATH|O_TMPFILE|RDONLY
// is accepted as an O_PATH handle (TMPFILE/access bits ignored), not EINVAL.
if uflags & O_TMPFILE == O_TMPFILE && uflags & 0b11 == O_RDONLY && uflags & O_PATH == 0 {
return Err(AxError::InvalidInput);
}

// Absolute path: man "If pathname is absolute, then dirfd is ignored."
// starry with_fs() unconditionally calls Directory::from_fd(dirfd),
// so invalid dirfd would EBADF before the abs-path shortcut applies.
// Same pattern as resolve_at (PR #605) / sys_fchownat (PR #588).
// Fixes bug-openat-abs-path-honors-invalid-dirfd.
let dirfd = if path.starts_with('/') {
AT_FDCWD as _
} else {
dirfd
};

let mode = mode & !current().as_thread().proc_data.umask();

let cred = current().as_thread().cred();
Expand Down
72 changes: 52 additions & 20 deletions os/arceos/modules/axfs-ng/src/highlevel/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,15 @@ impl OpenOptions {
fn _open(&self, loc: Location) -> VfsResult<OpenResult> {
let flags = self.to_flags()?;

// O_CREAT on an existing directory → EISDIR (Linux behavior;
// CREAT carries an implicit "create regular file" intent that
// conflicts with an existing directory regardless of access mode).
// Fixes bug-open-creat-on-existing-dir-no-eisdir.
// O_PATH path bypasses this — it doesn't actually open / mutate.
if self.create && loc.is_dir() && !self.path {
return Err(VfsError::IsADirectory);
}

if self.directory {
loc.check_is_dir()?;
}
Expand Down Expand Up @@ -246,6 +255,15 @@ impl OpenOptions {
return Err(VfsError::InvalidInput);
}

// Empty pathname → NotFound. man "ENOENT — O_CREAT is not set and
// the named file does not exist." resolve_parent("") would otherwise
// return cwd itself which lets open() succeed — wrong per POSIX.
// openat() does not accept AT_EMPTY_PATH; only specific *at calls do.
// Fixes bug-openat-empty-path-no-enoent.
if path.as_ref().as_str().is_empty() {
return Err(VfsError::NotFound);
}

let loc = match context.resolve_parent(path.as_ref()) {
Ok((parent, name)) => {
let mut loc = parent.open_file(
Expand All @@ -262,6 +280,13 @@ impl OpenOptions {
loc = context
.with_current_dir(parent)?
.try_resolve_symlink(loc, &mut 0)?;
} else if loc.node_type() == NodeType::Symlink && !self.path {
// O_NOFOLLOW + basename is a symlink + not O_PATH:
// man "If the trailing component (i.e., basename) of
// pathname is a symbolic link, then the open fails,
// with the error ELOOP."
// Fixes bug-open-nofollow-sym.
return Err(VfsError::FilesystemLoop);
}
loc
}
Expand All @@ -275,12 +300,18 @@ impl OpenOptions {
}

pub(crate) fn to_flags(&self) -> VfsResult<FileFlags> {
// Linux semantic: O_APPEND only adds APPEND bit; it does NOT promote
// read-only fd to read/write. (Previous code merged (true,_,true) →
// READ|WRITE|APPEND which silently upgraded RDONLY|APPEND to RW —
// see bug-open-rdonly-append-promotes-rw.)
Ok(match (self.read, self.write, self.append) {
(true, false, false) => FileFlags::READ,
(false, true, false) => FileFlags::WRITE,
(true, true, false) => FileFlags::READ | FileFlags::WRITE,
(false, _, true) => FileFlags::WRITE | FileFlags::APPEND,
(true, _, true) => FileFlags::READ | FileFlags::WRITE | FileFlags::APPEND,
(true, false, true) => FileFlags::READ | FileFlags::APPEND,
(false, true, true) => FileFlags::WRITE | FileFlags::APPEND,
(true, true, true) => FileFlags::READ | FileFlags::WRITE | FileFlags::APPEND,
(false, false, true) => FileFlags::APPEND, // RDONLY-equivalent + APPEND: pure status
(false, false, false) => return Err(VfsError::InvalidInput),
} | if self.path {
FileFlags::PATH
Expand All @@ -293,19 +324,12 @@ impl OpenOptions {
if !self.read && !self.write && !self.append {
return false;
}
match (self.write, self.append) {
(true, false) => {}
(false, false) => {
if self.truncate {
return false;
}
}
(_, true) => {
if self.truncate && !self.create_new {
return false;
}
}
}
// Linux multi-fs: RDONLY|TRUNC truncates the file (POSIX VERSIONS
// says effect is "unspecified", but most fs do truncate). Don't
// reject. Fixes bug-open-rdonly-trunc-einval.
// RDWR|APPEND|TRUNC is also explicitly allowed by Linux; the prior
// restriction "(_,true) && truncate && !create_new → false" was too
// strict. Fixes bug-open-append-trunc-einval.
true
}
}
Expand Down Expand Up @@ -903,15 +927,18 @@ pub struct File {
impl File {
/// Creates a new [`File`] from a [`FileBackend`] and access flags.
pub fn new(inner: FileBackend, flags: FileFlags) -> Self {
// man 2 open: "The file offset is set to the beginning of the file"
// — initial position is always 0, regardless of O_APPEND.
// O_APPEND only relocates the offset BEFORE EACH WRITE (handled in
// `write()` via the `access(FileFlags::APPEND)` branch). Setting
// initial position to EOF would break read() on RDONLY|APPEND
// (read sees EOF immediately) — see bug-open-rdonly-append-promotes-rw.
let position = if inner.location().flags().contains(NodeFlags::STREAM) {
None
} else {
Some(Mutex::new(if flags.contains(FileFlags::APPEND) {
inner.location().len().unwrap_or_default()
} else {
0
}))
Some(Mutex::new(0))
};
let _ = flags;
Self {
inner,
flags: AtomicU8::new(flags.bits()),
Expand Down Expand Up @@ -1028,6 +1055,11 @@ impl File {
{
self.access_flags.fetch_or(3, Ordering::AcqRel);
}
// WRITE bit is mandatory for any write path, regardless of whether
// APPEND is set. Otherwise O_RDONLY|O_APPEND fd would silently
// succeed writes (since access(APPEND) only checks the APPEND bit).
// Fixes bug-open-rdonly-append-promotes-rw (the part inside axfs).
self.access(FileFlags::WRITE)?;
if let Some(pos) = self.position.as_ref() {
let mut pos = pos.lock();
if let Ok(f) = self.access(FileFlags::APPEND) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
cmake_minimum_required(VERSION 3.20)
project(bug-open-append-trunc-einval C)
set(CMAKE_C_STANDARD 11)
set(CMAKE_C_STANDARD_REQUIRED ON)
set(CMAKE_C_EXTENSIONS ON)
add_executable(bug-open-append-trunc-einval src/main.c)
target_compile_options(bug-open-append-trunc-einval PRIVATE -Wall -Wextra -Werror)
install(TARGETS bug-open-append-trunc-einval RUNTIME DESTINATION usr/bin)
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* bug-open-append-trunc-einval: O_RDWR|O_APPEND|O_TRUNC on existing file
* must succeed (truncate then append).
*
* Linux behavior: O_RDWR + O_APPEND + O_TRUNC is a valid combination. open
* succeeds; the file is truncated to 0; subsequent writes append.
* StarryOS bug: axfs-ng/highlevel/file.rs:OpenOptions::is_valid() rejects
* `(_, true) => if self.truncate && !self.create_new { return false; }`
* when O_APPEND is set with O_TRUNC and CREAT_NEW isn't — returns -1 EINVAL.
*/
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>

int main(void)
{
const char *file = "/tmp/bug_append_trunc";
unlink(file);
int fd0 = open(file, O_CREAT | O_WRONLY, 0644);
if (fd0 < 0) { perror("setup"); return 1; }
write(fd0, "hello", 5);
close(fd0);

errno = 0;
int fd = open(file, O_RDWR | O_APPEND | O_TRUNC);
int ok = 0;
if (fd >= 0) {
struct stat st;
if (stat(file, &st) == 0 && st.st_size == 0) {
printf("PASS: open(O_RDWR|O_APPEND|O_TRUNC) -> fd=%d, size=0\n", fd);
ok = 1;
} else {
printf("FAIL: opened ok but size=%ld (expected 0)\n", (long)st.st_size);
}
close(fd);
} else {
printf("FAIL: open(O_RDWR|O_APPEND|O_TRUNC) -> -1 errno=%d (%s); expected fd>=0\n",
errno, strerror(errno));
}

unlink(file);
return ok ? 0 : 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
cmake_minimum_required(VERSION 3.20)
project(bug-open-creat-directory-einval C)
set(CMAKE_C_STANDARD 11)
set(CMAKE_C_STANDARD_REQUIRED ON)
set(CMAKE_C_EXTENSIONS ON)
add_executable(bug-open-creat-directory-einval src/main.c)
target_compile_options(bug-open-creat-directory-einval PRIVATE -Wall -Wextra -Werror)
install(TARGETS bug-open-creat-directory-einval RUNTIME DESTINATION usr/bin)
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* bug-open-creat-directory-einval: O_CREAT|O_DIRECTORY must fail with EINVAL.
*
* man 2 open implies (and Linux enforces): you cannot create a directory with
* open(); O_CREAT|O_DIRECTORY is rejected up front with EINVAL.
*
* Linux behavior: open(<anything>, O_CREAT|O_DIRECTORY) → -1 EINVAL
* StarryOS bug: doesn't reject the combo; either returns a valid fd (for
* existing directories) or lets path-resolution errors fire (ENOTDIR for
* regular files, EEXIST when O_EXCL also set on existing path).
*
* Root cause: starry's flags_to_options (kernel/src/syscall/fs/fd_ops.rs:26-63)
* doesn't validate that CREAT and DIRECTORY are mutually exclusive.
*/
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>

static int probe(const char *path, const char *desc)
{
errno = 0;
int fd = open(path, O_RDONLY | O_CREAT | O_DIRECTORY, 0644);
int ok = (fd == -1 && errno == EINVAL);
if (ok) {
printf("PASS: %s -> -1 EINVAL\n", desc);
} else {
printf("FAIL: %s -> fd=%d errno=%d (%s); expected -1 EINVAL\n",
desc, fd, errno, strerror(errno));
}
if (fd >= 0) close(fd);
return ok;
}

int main(void)
{
int ok = 1;

/* against existing regular file */
const char *file = "/tmp/bug_creat_dir_file";
unlink(file);
int fd = open(file, O_CREAT | O_WRONLY, 0644);
if (fd >= 0) close(fd);
ok &= probe(file, "existing regular file + O_CREAT|O_DIRECTORY");
unlink(file);

/* against existing directory */
const char *dir = "/tmp/bug_creat_dir_dir";
rmdir(dir);
mkdir(dir, 0755);
ok &= probe(dir, "existing directory + O_CREAT|O_DIRECTORY");
rmdir(dir);

/* against absent path */
const char *absent = "/tmp/bug_creat_dir_absent_xyz";
unlink(absent);
ok &= probe(absent, "absent path + O_CREAT|O_DIRECTORY");
unlink(absent);

return ok ? 0 : 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
cmake_minimum_required(VERSION 3.20)
project(bug-open-creat-on-existing-dir-no-eisdir C)
set(CMAKE_C_STANDARD 11)
set(CMAKE_C_STANDARD_REQUIRED ON)
set(CMAKE_C_EXTENSIONS ON)
add_executable(bug-open-creat-on-existing-dir-no-eisdir src/main.c)
target_compile_options(bug-open-creat-on-existing-dir-no-eisdir PRIVATE -Wall -Wextra -Werror)
install(TARGETS bug-open-creat-on-existing-dir-no-eisdir RUNTIME DESTINATION usr/bin)
Loading