[llvm][clang] Sandbox filesystem reads #165350
Conversation
jansvoboda11
force-pushed
the
sandboxing-v2
branch
from
a9c9c28 to
b062e35
Compare
jansvoboda11
force-pushed
the
sandboxing-v2
branch
from
b062e35 to
8e49476
Compare
| } | ||
|
|
||
| std::error_code is_other(const Twine &Path, bool &Result) { | ||
| sandbox::violationIfEnabled(); |
There was a problem hiding this comment.
Won't we catch these via status being a violation? Similarly for the other APIs here (e.g. openFileForRead).
There was a problem hiding this comment.
Yes, but I wanted to explicitly mark each API we expect to cause a sandbox violation. This is still conceptually an API that's required to touch the real FS. If the implementation ever changes from calling sys::fs::status() to calling status() directly, I still want this to remain a violation. Same goes for the other APIs.
There was a problem hiding this comment.
It seems hard to be completely safe against any possible future change like that. For example, there are a bunch of inline functions in FileSystem.h that could theoretically be refactored in the future without remembering to trigger a violation (most of them call a non-inline version that you have a sandbox call in, some call fs::status directly).
There was a problem hiding this comment.
That's true, my approach isn't bulletproof either. Would you prefer if we only triggered sandbox violations the core low-level functions and left stuff like is_other() alone? I don't have a strong preference here.
There was a problem hiding this comment.
That's my weak preference, but I won't block the PR over this if anyone disagrees. It is a (tiny) perf win to only check it once, but I doubt it matters.
2 participants
This PR introduces a new mechanism for enforcing a sandbox around filesystem reads coming from the compiler. A fatal error is raised whenever the
llvm::sys::fs,llvm::MemoryBuffer::getFile*()APIs get used directly instead of going through the "blessed" virtual interface ofllvm::vfs::FileSystem.