[clang] Prevent sandbox violations in CrossTranslationUnitContext

[jansvoboda11]

This uses the VFS to load a file instead of using MemoryBuffer::getBufferForFile() directly to avoid sandbox violation. Sandbox is then disabled for CreateASTUnitFromCommandLine() which invokes the driver which is not expected to be free of sandbox violations.

[github-actions]

🪟 Windows x64 Test Results

• 53380 tests passed
• 2216 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🐧 Linux x64 Test Results

• 112578 tests passed
• 4602 tests skipped

✅ The build succeeded and all tests passed.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

This PR propagates the VFS where necessary from CrossTranslationUnitContext::ASTLoader to avoid sandbox violations.

---

Full diff: https://github.com/llvm/llvm-project/pull/175097.diff

1 Files Affected:

• (modified) clang/lib/CrossTU/CrossTranslationUnit.cpp (+4-1)

diff --git a/clang/lib/CrossTU/CrossTranslationUnit.cpp b/clang/lib/CrossTU/CrossTranslationUnit.cpp
index a3fc2cf6bfb3c..7496deb74cb67 100644
--- a/clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ b/clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -24,6 +24,7 @@
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/IOSandbox.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/YAMLParser.h"
@@ -620,6 +621,8 @@ CrossTranslationUnitContext::ASTLoader::loadFromSource(
   auto Diags = llvm::makeIntrusiveRefCnt<DiagnosticsEngine>(DiagID, *DiagOpts,
                                                             DiagClient);
 
+  // This runs the driver which isn't expected to be free of sandbox violations.
+  auto BypassSandbox = llvm::sys::sandbox::scopedDisable();
   return CreateASTUnitFromCommandLine(
       CommandLineArgs.begin(), (CommandLineArgs.end()),
       CI.getPCHContainerOperations(), DiagOpts, Diags,
@@ -710,7 +713,7 @@ llvm::Error CrossTranslationUnitContext::ASTLoader::lazyInitInvocationList() {
     return llvm::make_error<IndexError>(PreviousParsingResult);
 
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> FileContent =
-      llvm::MemoryBuffer::getFile(InvocationListFilePath);
+      CI.getVirtualFileSystem().getBufferForFile(InvocationListFilePath);
   if (!FileContent) {
     PreviousParsingResult = index_error_code::invocation_list_file_not_found;
     return llvm::make_error<IndexError>(PreviousParsingResult);

[jansvoboda11]

This fixes these tests with sandboxing enabled:

Clang.Analysis/ctu-on-demand-parsing.c
Clang.Analysis/ctu-on-demand-parsing.cpp

[benlangmuir]

The change here LGTM, but do you think this deserves a FIXME? I'm not clear whether this is okay or not long term. From a VFS usage perspective calling back to the driver may or may not be okay if you don't pass in a VFS. From a caching perspective it would need special handling.

[jansvoboda11]

I'm not sure. We already have a sandbox disablement due to the driver here:

llvm-project/clang/tools/driver/cc1gen_reproducer_main.cpp

Lines 119 to 120 in 5c3f02c

	// The driver is not expected to be free of sandbox violations.
	auto BypassSandbox = llvm::sys::sandbox::scopedDisable();

and that doesn't have a FIXME. I'm not sure how actionable these FIXMEs would be, since the driver performs lots of FS operations and some don't have an equivalent on the VFS layer (access() comes to mind). Currently there'd be little upside in resolving the FIXMEs (even though it makes sense conceptually).

I don't have a strong opinion, but if we decide to go forward with the FIXMEs, wouldn't it make sense to put them on the driver and have the sandbox disablement somewhere in the clangDriver library?

[benlangmuir]

I'm not really concerned about the driver itself, that is when it is parsing arguments for and then coordinating the frontend execution, it's the possibility of the driver being called back by the frontend specifically that seems (potentially) problematic if it allows the frontend to indirectly depend on unsandboxed FS access. Probably this is more of a caching issue than a general VFS issue, and it would need special handling to cache correctly anyway, so maybe it's fine to ignore it for now?