[clang][AST][ASTMerge] prevent AST nodes from being deallocated early

[jcsxky]

This patch aims to fix error in ast-merge to new ast file.
ASTUnit is put in for body and AST nodes would be deallocated by allocator. Using these nodes later would lead to use after free bug.
Debug the issue can prove it. Address interval (local from 0x3a9a00 to 0x3aaa00) allocated by allocator contains a IdentifierInfo variable (local address:0x3aa190) whose address is freed early.
As system header like stdio.h or math.h can't be put into test, it's hard to add testcase. Could anyone give me some guidance? Thanks in advance!

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

This patch aims to fix error in ast-merge to new ast file.
ASTUnit is put in for body and AST nodes would be deallocated by allocator. Using these nodes later would lead to use after free bug.
Debug the issue can prove it. Address interval (local from 0x3a9a00 to 0x3aaa00) allocated by allocator contains a IdentifierInfo variable (local address:0x3aa190) whose address is freed early.
As system header like stdio.h or math.h can't be put into test, it's hard to add testcase. Could anyone give me some guidance? Thanks in advance!

---

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

1 Files Affected:

• (modified) clang/lib/Frontend/ASTMerge.cpp (+8-5)

diff --git a/clang/lib/Frontend/ASTMerge.cpp b/clang/lib/Frontend/ASTMerge.cpp
index 057ea4fd5bb3518..94706ade4c876a2 100644
--- a/clang/lib/Frontend/ASTMerge.cpp
+++ b/clang/lib/Frontend/ASTMerge.cpp
@@ -5,14 +5,15 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-#include "clang/Frontend/ASTUnit.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/ASTImporter.h"
 #include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
+#include "llvm/ADT/SmallVector.h"
 
 using namespace clang;
 
@@ -40,24 +41,26 @@ void ASTMergeAction::ExecuteAction() {
       DiagIDs(CI.getDiagnostics().getDiagnosticIDs());
   auto SharedState = std::make_shared<ASTImporterSharedState>(
       *CI.getASTContext().getTranslationUnitDecl());
+  llvm::SmallVector<std::unique_ptr<ASTUnit>> Units(ASTFiles.size());
   for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
     IntrusiveRefCntPtr<DiagnosticsEngine>
         Diags(new DiagnosticsEngine(DiagIDs, &CI.getDiagnosticOpts(),
                                     new ForwardingDiagnosticConsumer(
                                           *CI.getDiagnostics().getClient()),
                                     /*ShouldOwnClient=*/true));
-    std::unique_ptr<ASTUnit> Unit = ASTUnit::LoadFromASTFile(
+    Units[I] = ASTUnit::LoadFromASTFile(
         ASTFiles[I], CI.getPCHContainerReader(), ASTUnit::LoadEverything, Diags,
         CI.getFileSystemOpts(), CI.getHeaderSearchOptsPtr(), false);
 
-    if (!Unit)
+    if (!Units[I])
       continue;
 
     ASTImporter Importer(CI.getASTContext(), CI.getFileManager(),
-                         Unit->getASTContext(), Unit->getFileManager(),
+                         Units[I]->getASTContext(), Units[I]->getFileManager(),
                          /*MinimalImport=*/false, SharedState);
 
-    TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
+    TranslationUnitDecl *TU =
+        Units[I]->getASTContext().getTranslationUnitDecl();
     for (auto *D : TU->decls()) {
       // Don't re-import __va_list_tag, __builtin_va_list.
       if (const auto *ND = dyn_cast<NamedDecl>(D))

[ChuanqiXu9]

Debug the #72783 can prove it. Address interval (local from 0x3a9a00 to 0x3aaa00) allocated by allocator contains a IdentifierInfo variable (local address:0x3aa190) whose address is freed early.

In this case, it looks better to extract the use-after-free variable only instead of extracting the whole ASTUnit.

As system header like stdio.h or math.h can't be put into test, it's hard to add testcase. Could anyone give me some guidance? Thanks in advance!

Generally, we need to reduce them in this case. e.g., we need to preprocess them, and remove unncessary parts until we can't. It is time consuming but it is worthy.

[jcsxky]

Debug the #72783 can prove it. Address interval (local from 0x3a9a00 to 0x3aaa00) allocated by allocator contains a IdentifierInfo variable (local address:0x3aa190) whose address is freed early.

In this case, it looks better to extract the use-after-free variable only instead of extracting the whole ASTUnit.

• From my local debugging, it's a IdentifierInfo type variable which is freed by allocator. The variable is subnode of AST. Thanks to ASTUnit is out of scope, some related memory is freed (which is allocated by SpecificBumpPtrAllocator) as destructor called and we can't extract only IdentifierInfo type variable.

As system header like stdio.h or math.h can't be put into test, it's hard to add testcase. Could anyone give me some guidance? Thanks in advance!

Generally, we need to reduce them in this case. e.g., we need to preprocess them, and remove unncessary parts until we can't. It is time consuming but it is worthy.

• Small piece of code can't reproduce the crash. The crash is caused by growing of size of OnDiskChainedHashTableGenerator when add IdentifierInfo type variables. As mentioned in the issue, when remove header file, it runs OK. Small-scale code wouldn't cause resize of OnDiskChainedHashTableGenerator and memory allocation.

[balazske]

You have found that reason for the crash is that references to IdentifierInfo are remaining in OnDiskChainedHashTableGenerator and previously deallocated by ASTUnit destruction? In this case why is the ASTUnit (or something in it, probably ASTContext) the owner of the data, and not OnDiskChainedHashTableGenerator?
By using ASTImporter it is possible that it moves data from the "old" AST to the "new" without copy (if yes this is a bug) and such a situation can cause memory errors.

[jcsxky]

You have found that reason for the crash is that references to IdentifierInfo are remaining in OnDiskChainedHashTableGenerator and previously deallocated by ASTUnit destruction? In this case why is the ASTUnit (or something in it, probably ASTContext) the owner of the data, and not OnDiskChainedHashTableGenerator? By using ASTImporter it is possible that it moves data from the "old" AST to the "new" without copy (if yes this is a bug) and such a situation can cause memory errors.

Thanks for your remind! Indeed, root cause of the issue is ASTImporter and I have checked the code. IdentifierInfo of attribute should set with ToASTContext instead of copying from FromASTContext.

New patch: [clang][ASTImporter] IdentifierInfo of Attribute should be set using 'ToASTContext'

[yangxili2023]

Thank you everybody! The problem is solved.