[clang-repl] Fixing vulnerabilities with respect to orc runtime #165852
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Anutosh Bhat (anutosh491) ChangesI was trying to play around with clang-repl out of process on my linux machine and I came across these vulnerabilities. Full diff: https://github.com/llvm/llvm-project/pull/165852.diff 2 Files Affected:
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index cde354c9cd8d1..21e03bbfd532a 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -421,9 +421,6 @@ Interpreter::getOrcRuntimePath(const driver::ToolChain &TC) {
llvm::Expected<std::unique_ptr<Interpreter>>
Interpreter::create(std::unique_ptr<CompilerInstance> CI, JITConfig Config) {
- llvm::Error Err = llvm::Error::success();
-
- std::unique_ptr<llvm::orc::LLJITBuilder> JB;
if (Config.IsOutOfProcess) {
const TargetInfo &TI = CI->getTarget();
@@ -453,6 +450,9 @@ Interpreter::create(std::unique_ptr<CompilerInstance> CI, JITConfig Config) {
}
}
+ llvm::Error Err = llvm::Error::success();
+ std::unique_ptr<llvm::orc::LLJITBuilder> JB;
+
auto Interp = std::unique_ptr<Interpreter>(new Interpreter(
std::move(CI), Err, std::move(JB), /*Consumer=*/nullptr, Config));
if (auto E = std::move(Err))
diff --git a/clang/tools/clang-repl/ClangRepl.cpp b/clang/tools/clang-repl/ClangRepl.cpp
index c7879422cd7df..c86a1314ac026 100644
--- a/clang/tools/clang-repl/ClangRepl.cpp
+++ b/clang/tools/clang-repl/ClangRepl.cpp
@@ -309,6 +309,7 @@ int main(int argc, const char **argv) {
clang::Interpreter::JITConfig Config;
Config.IsOutOfProcess = !OOPExecutor.empty() || !OOPExecutorConnect.empty();
Config.OOPExecutor = OOPExecutor;
+ Config.OrcRuntimePath = OrcRuntimePath;
auto SizeOrErr = getSlabAllocSize(SlabAllocateSizeString);
if (!SizeOrErr) {
llvm::logAllUnhandledErrors(SizeOrErr.takeError(), llvm::errs(), "error: ");
|
|
No 3 : (proposing a fix through upcoming commits) I see that the orc runtime on a linux machine would end up somewhere here (as in my case) But on a mac machine, it might end up here (as in @kr-2003 's case) So we have access to these two location in the llvm-project/clang/lib/Interpreter/Interpreter.cpp Lines 395 to 399 in 8ea447b
And the orc runtime can be present in one of these 2 locations but llvm-project/clang/lib/Interpreter/Interpreter.cpp Lines 405 to 415 in 8ea447b We need to update this to prioritise both ! |
|
The latest commit respects both paths
|
Sadly I haven't been able to find a way here :\ As we're making changes to getORCRuntimePath the organic path should have been getting rid of the stub function that the test uses while creating the interpreter llvm-project/clang/unittests/Interpreter/OutOfProcessInterpreterTests.cpp Lines 137 to 138 in 6ad25c5 We should have been able to simply use the public function, getOrcRuntimePath from the interpreter class here !! But @kr-2003 educated me that he already tried this out. He says That being said, we can check the correctness of this test simply by running Without the patch on current master With the patch on current master Fails if we force a dummy failure. for eg |
|
If the above looks convincing, maybe let's take the patch in and keep exploring better ways to test these utility function like |
|
Hey @vgvassilev , As discussed with @Vipul-Cariappa and @kr-2003 , we think testing path based helper utilities like Testing the out-of-process.cpp file through llvm-lit was the most organic approach we could think of as of now as mentioned here #165852 (comment) Do you think this is good enough to take this in for now |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Addressed the suggestions above. Trying to forcefully mess up the path in my case shows me this Which is covering both paths as we want. |
|
Thanks for the review. Merging ! |
…#165852) Fixed `getORCRuntimePath` function to respect `getCompilerRTPath` and `getRuntimePath`
3 participants
I was trying to play around with clang-repl out of process on my linux machine and I came across these vulnerabilities.