posix: make posix access modes enum fields#753
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.
| #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.
There was a problem hiding this comment.
I don't think we have to avoid breaking changes since we already have broken multiple things on the master branch and recompilation is needed anyway. The "ABI stability" policy will be in effect since release 3.4. IMHO we can change the values.
There was a problem hiding this comment.
IMHO we can change it later, but we should disassociate this change from WAMR patchset.
This one might require a wider evaluation of what might break.
For example, LWIP (because it is a GNU project) interprets access modes as bitfields, I don't have knowledge on our implementation, but intuition tells me that there might be places where this change will break other components.
For merging a port which is mostly done, this might be too much effort for now, we should postpone doing this.
3 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