[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-28 Thread Old Head Music via cfe-commits 

yangxili2023 wrote:

Thank you everybody! The problem is solved.

https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-23 Thread Qizhi Hu via cfe-commits 

https://github.com/jcsxky closed https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-23 Thread Qizhi Hu via cfe-commits 

jcsxky wrote:

> 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`.




https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-23 Thread Balázs Kéri via cfe-commits 

balazske wrote:

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.

https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-23 Thread Qizhi Hu via cfe-commits 

jcsxky wrote:

> > 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 
variable. As mentioned in the 
[issue](https://github.com/llvm/llvm-project/issues/72783), when remove header 
file, it runs OK. Small-scale code wouldn't cause resize of 
`OnDiskChainedHashTableGenerator`


https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-22 Thread Chuanqi Xu via cfe-commits 

ChuanqiXu9 wrote:

> Debug the https://github.com/llvm/llvm-project/issues/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.

https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-22 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)


Changes

This patch aims to fix [error in ast-merge to new ast 
file](https://github.com/llvm/llvm-project/issues/72783).
`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](https://github.com/llvm/llvm-project/issues/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.
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
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(
   *CI.getASTContext().getTranslationUnitDecl());
+  llvm::SmallVector> Units(ASTFiles.size());
   for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
 IntrusiveRefCntPtr
 Diags(new DiagnosticsEngine(DiagIDs, (),
 new ForwardingDiagnosticConsumer(
   *CI.getDiagnostics().getClient()),
 /*ShouldOwnClient=*/true));
-std::unique_ptr 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(D))

``




https://github.com/llvm/llvm-project/pull/73096
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][AST][ASTMerge] prevent AST nodes from being deallocated early (PR #73096)

2023-11-22 Thread Qizhi Hu via cfe-commits 

https://github.com/jcsxky created 
https://github.com/llvm/llvm-project/pull/73096

This patch aims to fix [error in ast-merge to new ast 
file](https://github.com/llvm/llvm-project/issues/72783).
`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](https://github.com/llvm/llvm-project/issues/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.
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!

>From 0c3b47b351eb90495d635fd37845d5c7da869b6e Mon Sep 17 00:00:00 2001
From: huqizhi 
Date: Wed, 22 Nov 2023 16:41:42 +0800
Subject: [PATCH] [clang][AST][ASTMerge] prevent AST nodes from being
 deallocated early

---
 clang/lib/Frontend/ASTMerge.cpp | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

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(
   *CI.getASTContext().getTranslationUnitDecl());
+  llvm::SmallVector> Units(ASTFiles.size());
   for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
 IntrusiveRefCntPtr
 Diags(new DiagnosticsEngine(DiagIDs, (),
 new ForwardingDiagnosticConsumer(
   *CI.getDiagnostics().getClient()),
 /*ShouldOwnClient=*/true));
-std::unique_ptr 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(D))

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