ExtractVirtualFileSystemResources does not set correct permissions#46
ExtractVirtualFileSystemResources does not set correct permissions#46ih0r-d wants to merge 2 commits intooracle:mainfrom
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
ih0r-d
changed the title
ih0r-d
force-pushed
the
feature/43-extract-vfs-resources-permissions
branch
2 times, most recently
from
01f9a4f to
7cee664
Compare
...aalvm.python.embedding/src/main/java/org/graalvm/python/embedding/VirtualFileSystemImpl.java
Outdated
Show resolved
Hide resolved
ih0r-d
force-pushed
the
feature/43-extract-vfs-resources-permissions
branch
3 times, most recently
from
5e340eb to
0c7bdc9
Compare
ih0r-d
force-pushed
the
feature/43-extract-vfs-resources-permissions
branch
3 times, most recently
from
84fed6e to
903b332
Compare
…ction # Conflicts: # org.graalvm.python.embedding/src/main/java/org/graalvm/python/embedding/VirtualFileSystemImpl.java
ih0r-d
force-pushed
the
feature/43-extract-vfs-resources-permissions
branch
from
903b332 to
cc9b62c
Compare
| String.format("execute access should not be possible for non-executable file '%s'", p)); | ||
| } | ||
|
|
||
| throw securityException("VFS.checkAccess", |
There was a problem hiding this comment.
Why this change? I think that main goal is that the permissions are applied when the VFS is extracted to the real filesystem, but when the VFS is used at runtime as virtualized filesystem, I think we should still present all the files as non-executable, because there's no way to actually execute them.
|
|
||
| // v1 format: only absolute path | ||
| if (line.startsWith("/")) { | ||
| Set<PosixFilePermission> permissions = isExecutable(line) |
There was a problem hiding this comment.
With the v1 format we should have the same (broken) permissions as before to stay fully compatible.
| dirEntry = de; | ||
| } else if (genericEntry == null) { | ||
| dirEntry = new DirEntry(dir); | ||
| dirEntry = new DirEntry(dir, DEFAULT_DIR_PERMISSIONS); |
There was a problem hiding this comment.
Here we create intermediate directories with default permissions if they are missing, but what if an intermediate directory is just listed later in the fileslist.txt with different than default permissions? E.g., when
dir xxxx /GRAALPY-VFS/foo/
dir xxxx /GRAALPY-VFS/foo/otherdir/
file xxxx /GRAALPY-VFS/foo/otherdir/hello.txt
is listed as
file xxxx /GRAALPY-VFS/foo/otherdir/hello.txt
dir xxxx /GRAALPY-VFS/foo/
dir xxxx /GRAALPY-VFS/foo/otherdir/
| throw new IllegalArgumentException("Invalid fileslist entry (expected: <type> <mode> <path>): " + line); | ||
| } | ||
|
|
||
| EntryType type = switch (parts[0]) { |
There was a problem hiding this comment.
We already have convention that directories must end with PLATFORM_SEPARATOR - otherwise it is a file. No strong opinion, but I would find it simpler to keep this convention rather than to have extra switch here.
|
Thanks, @ih0r-d, for your contribution! I left some inline comments. Apart from that:
|
3 participants
Description
Permissions are now stored as VFS metadata and applied on extraction.
This fixes non-executable binaries after extractResources.
Type of change
Related to #43
What is changed
Updated
VirtualFileSystemImplto support explicit POSIX file permissions as filesystem metadata instead of relying on implicit or hardcoded executable heuristics.The VFS now:
fileslist,This makes VFS behavior deterministic and aligned with real filesystem semantics.
How Has This Been Tested?
VirtualFileSystemunit tests locally after updating them to reflect permission-based behavior.Test Configuration:
Checklist