[PATCH] D90599: Fix a leak in `ASTUnit::LoadFromCommandLine()` wehn compiler invocation fails

[Boris Staletic via Phabricator via cfe-commits]

bstaletic created this revision.
bstaletic added a reviewer: aprantl.
bstaletic added projects: clang, clang-c.
Herald added a subscriber: cfe-commits.
bstaletic requested review of this revision.

1. Here 
<https://github.com/llvm/llvm-project/blob/master/clang/tools/libclang/CIndex.cpp#L3567-L3571>
 we allocate buffers to hold the contents of unsaved files.
2. The `RemappedFiles` vector is then passed to 
`ASTUnit::LoadFromCommandLine()` as an `ArrayRef`, here 
<https://github.com/llvm/llvm-project/blob/master/clang/tools/libclang/CIndex.cpp#L3632>
3. In ASTUnit::LoadFromCommandLine() 
<https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/ASTUnit.cpp#L1735>
 we try to make a compiler invocation and exit early if that fails 
<https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/ASTUnit.cpp#L1758-L1761>.
4. If we did exit early from that function, both Unit and UnitErr are nullptr 
<https://github.com/llvm/llvm-project/blob/master/clang/tools/libclang/CIndex.cpp#L3641-L3642>,
 making the function exit without ever cleaning up the memory allocations from 
step 1.

Notes:

1. The function allocating all those `RemappedFile` objects is not actually the 
same as the one deallocating, in the happy case.

1.1. I did not figure out where exactly does it get deleted, because the above 
seems to be enough data for now.

2. My attempted solution is to iterate over remapped files and deallocate in 
this block 
<https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/ASTUnit.cpp#L1760-L1761>,
 but it seems odd to deallocate through `ArrayRef`.

Fixes https://bugs.llvm.org/show_bug.cgi?id=47832


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90599

Files:
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===================================================================
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1758,7 +1758,11 @@
     CI = createInvocationFromCommandLine(
         llvm::makeArrayRef(ArgBegin, ArgEnd), Diags, VFS);
     if (!CI)
+    {
+      for (const auto &RF : RemappedFiles)
+        delete RF.second;
       return nullptr;
+    }
   }
 
   // Override any files that need remapping



Index: clang/lib/Frontend/ASTUnit.cpp
===================================================================
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1758,7 +1758,11 @@
     CI = createInvocationFromCommandLine(
         llvm::makeArrayRef(ArgBegin, ArgEnd), Diags, VFS);
     if (!CI)
+    {
+      for (const auto &RF : RemappedFiles)
+        delete RF.second;
       return nullptr;
+    }
   }
 
   // Override any files that need remapping

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits