Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
oI0ck
force-pushed
the
michal.lach/accmode_enum
branch
from
2d22040 to
8b84664
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the POSIX file access mode constants (O_RDONLY, O_WRONLY, O_RDWR) from bitfield flags to enum-style values and introduces a new O_ACCMODE mask. While this aligns with POSIX standards, the change is critical as it breaks existing bitwise logic throughout the codebase. Specifically, since O_RDONLY is now 0, existing checks for read-only status will fail, potentially allowing unauthorized writes or truncations. A full audit and update of all access mode checks using O_ACCMODE is required to prevent these regressions.
include/posix-fcntl.h
Outdated
| #define O_RDONLY 0x00000U | ||
| #define O_WRONLY 0x00001U | ||
| #define O_RDWR 0x00002U |
There was a problem hiding this comment.
Changing these access modes from bitfields to enum-style values has consequences in other parts of the code that are not updated in this PR. This can lead to critical bugs.
-
Writing to read-only files: Checks like
(f->status & O_RDONLY) != 0Uinposix_write()(posix/posix.c:786) will always be false becauseO_RDONLYis now 0. This allows writing to files opened as read-only. This is a critical issue. -
Truncating read-only files: Similarly,
(f->status & O_RDONLY) == 0Uinposix_ftruncate()(posix/posix.c:1278) will always be true, allowing truncation of read-only files. -
Incorrect pipe creation: Expressions like
O_RDONLY | O_WRONLYnow evaluate toO_WRONLY. This is used inposix_pipe()(posix/posix.c:981) and likely breaks pipe creation, which should be read/write. It should probably beO_RDWR.
All checks on access modes should be updated to use O_ACCMODE. For example:
- To check for read-only:
(flags & O_ACCMODE) == O_RDONLY - To check for not read-only:
(flags & O_ACCMODE) != O_RDONLY - To check for write-only:
(flags & O_ACCMODE) == O_WRONLY
Please audit all usages of O_RDONLY, O_WRONLY, and O_RDWR in the codebase and update them accordingly.
oI0ck
force-pushed
the
michal.lach/accmode_enum
branch
2 times, most recently
from
5f8b0bb to
602aae3
Compare
Most POSIX conformant/compilant systems treat O_ACCMODE fields as enum fields, not bitfields, which carries on to their usage semantics, making it impossible to repesent states when modes are not mutually exclusive. JIRA: RTOS-1088
oI0ck
force-pushed
the
michal.lach/accmode_enum
branch
from
602aae3 to
a41e233
Compare
Unit Test Results9 508 tests - 45 8 770 ✅ - 191 58m 1s ⏱️ + 4m 52s For more details on these failures, see this check. Results for commit a41e233. ± Comparison against base commit 09ea2db. This pull request removes 54 and adds 9 tests. Note that renamed tests count towards both. |
| #define O_RDONLY 0x00001U | ||
| #define O_WRONLY 0x00002U | ||
| #define O_RDWR 0x00004U | ||
| /* |
There was a problem hiding this comment.
technically we could decide to not change the numeric values of these defines - that way we will keep binary compatibility (so the change would not be breaking)?
There was a problem hiding this comment.
Technically, yes, though, having none of these bits set is an invalid state, which is why I marginally prefer starting with 0, although even with constants starting from 0, one could have a 11 state, which is also incorrect, so that might not matter as much as I'd like it to.
2 participants
Most POSIX conformant/compilant systems treat O_ACCMODE fields as enum fields, not bitfields, which carries on to their usage semantics, making it impossible to repesent states when modes are not mutually exclusive.
JIRA: RTOS-1088
Types of changes
How Has This Been Tested?
ia32-generic-qemu,armv7a9-zynq7000-qemu.Checklist:
Special treatment