[Clang] Fixed double finally block execution #146796

yasster · 2025-07-02T22:30:41Z

Clang's SEH code generation could execute __finally blocks twice when exceptions were raised outside the finally block, leading to potential double-free and memory corruption issues. The root cause was the compiler generates IR that allowed finally blocks to be reached through normal paths and exception paths, which can cause cleanup path to execute multiple times.

github-actions · 2025-07-02T22:31:00Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-07-02T22:31:31Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Yassine (yasster)

Changes

Clang's SEH code generation could execute __finally blocks twice when exceptions were raised outside the finally block, leading to potential double-free and memory corruption issues. The root cause was the compiler generates IR that allowed finally blocks to be reached through normal paths and exception paths, which can cause cleanup path to execute multiple times.

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGException.cpp (+37-8)
(added) clang/test/CodeGen/seh-finally-double-execute.c (+34)

diff --git a/clang/lib/CodeGen/CGException.cpp b/clang/lib/CodeGen/CGException.cpp
index ad138b9876e8c..ab4086716cc1c 100644
--- a/clang/lib/CodeGen/CGException.cpp
+++ b/clang/lib/CodeGen/CGException.cpp
@@ -1368,14 +1368,24 @@ namespace {
     llvm::FunctionCallee EndCatchFn;
     llvm::FunctionCallee RethrowFn;
     llvm::Value *SavedExnVar;
+    llvm::Value *FinallyExecutedFlag;
 
     PerformFinally(const Stmt *Body, llvm::Value *ForEHVar,
                    llvm::FunctionCallee EndCatchFn,
-                   llvm::FunctionCallee RethrowFn, llvm::Value *SavedExnVar)
+                   llvm::FunctionCallee RethrowFn, llvm::Value *SavedExnVar,
+                   llvm::Value *FinallyExecutedFlag)
         : Body(Body), ForEHVar(ForEHVar), EndCatchFn(EndCatchFn),
-          RethrowFn(RethrowFn), SavedExnVar(SavedExnVar) {}
+          RethrowFn(RethrowFn), SavedExnVar(SavedExnVar),
+          FinallyExecutedFlag(FinallyExecutedFlag) {}
 
     void Emit(CodeGenFunction &CGF, Flags flags) override {
+      // Only execute the finally block if it hasn't already run.
+      llvm::BasicBlock *RunFinallyBB = CGF.createBasicBlock("finally.run");
+      llvm::BasicBlock *SkipFinallyBB = CGF.createBasicBlock("finally.skip");
+      llvm::Value *AlreadyExecuted = CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
+      CGF.Builder.CreateCondBr(AlreadyExecuted, SkipFinallyBB, RunFinallyBB);
+      CGF.EmitBlock(RunFinallyBB);
+      CGF.Builder.CreateFlagStore(true, FinallyExecutedFlag);
       // Enter a cleanup to call the end-catch function if one was provided.
       if (EndCatchFn)
         CGF.EHStack.pushCleanup<CallEndCatchForFinally>(NormalAndEHCleanup,
@@ -1429,6 +1439,7 @@ namespace {
       // Now make sure we actually have an insertion point or the
       // cleanup gods will hate us.
       CGF.EnsureInsertPoint();
+      CGF.EmitBlock(SkipFinallyBB);
     }
   };
 } // end anonymous namespace
@@ -1478,10 +1489,12 @@ void CodeGenFunction::FinallyInfo::enter(CodeGenFunction &CGF, const Stmt *body,
   ForEHVar = CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), "finally.for-eh");
   CGF.Builder.CreateFlagStore(false, ForEHVar);
 
-  // Enter a normal cleanup which will perform the @finally block.
+  // Allocate a flag to ensure the finally block is only executed once.
+  llvm::Value *FinallyExecutedFlag = CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), "finally.executed");
+  CGF.Builder.CreateFlagStore(false, FinallyExecutedFlag);
   CGF.EHStack.pushCleanup<PerformFinally>(NormalCleanup, body,
                                           ForEHVar, endCatchFn,
-                                          rethrowFn, SavedExnVar);
+                                          rethrowFn, SavedExnVar, FinallyExecutedFlag);
 
   // Enter a catch-all scope.
   llvm::BasicBlock *catchBB = CGF.createBasicBlock("finally.catchall");
@@ -1724,10 +1737,18 @@ void CodeGenFunction::VolatilizeTryBlocks(
 namespace {
 struct PerformSEHFinally final : EHScopeStack::Cleanup {
   llvm::Function *OutlinedFinally;
-  PerformSEHFinally(llvm::Function *OutlinedFinally)
-      : OutlinedFinally(OutlinedFinally) {}
+  llvm::Value *FinallyExecutedFlag;
+  PerformSEHFinally(llvm::Function *OutlinedFinally, llvm::Value *FinallyExecutedFlag)
+      : OutlinedFinally(OutlinedFinally), FinallyExecutedFlag(FinallyExecutedFlag) {}
 
   void Emit(CodeGenFunction &CGF, Flags F) override {
+    // Only execute the finally block if it hasn't already run.
+    llvm::BasicBlock *RunFinallyBB = CGF.createBasicBlock("finally.run");
+    llvm::BasicBlock *SkipFinallyBB = CGF.createBasicBlock("finally.skip");
+    llvm::Value *AlreadyExecuted = CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
+    CGF.Builder.CreateCondBr(AlreadyExecuted, SkipFinallyBB, RunFinallyBB);
+    CGF.EmitBlock(RunFinallyBB);
+    CGF.Builder.CreateFlagStore(true, FinallyExecutedFlag);
     ASTContext &Context = CGF.getContext();
     CodeGenModule &CGM = CGF.CGM;
 
@@ -1769,6 +1790,8 @@ struct PerformSEHFinally final : EHScopeStack::Cleanup {
 
     auto Callee = CGCallee::forDirect(OutlinedFinally);
     CGF.EmitCall(FnInfo, Callee, ReturnValueSlot(), Args);
+    
+    CGF.EmitBlock(SkipFinallyBB);
   }
 };
 } // end anonymous namespace
@@ -2164,7 +2187,10 @@ llvm::Value *CodeGenFunction::EmitSEHAbnormalTermination() {
 
 void CodeGenFunction::pushSEHCleanup(CleanupKind Kind,
                                      llvm::Function *FinallyFunc) {
-  EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc);
+  // Allocate a flag to ensure the finally block is only executed once.
+  llvm::Value *FinallyExecutedFlag = CreateTempAlloca(Builder.getInt1Ty(), "finally.executed");
+  Builder.CreateFlagStore(false, FinallyExecutedFlag);
+  EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc, FinallyExecutedFlag);
 }
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
@@ -2175,8 +2201,11 @@ void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
     llvm::Function *FinallyFunc =
         HelperCGF.GenerateSEHFinallyFunction(*this, *Finally);
 
+    // Allocate a flag to ensure the finally block is only executed once.
+    llvm::Value *FinallyExecutedFlag = CreateTempAlloca(Builder.getInt1Ty(), "finally.executed");
+    Builder.CreateFlagStore(false, FinallyExecutedFlag);
     // Push a cleanup for __finally blocks.
-    EHStack.pushCleanup<PerformSEHFinally>(NormalAndEHCleanup, FinallyFunc);
+    EHStack.pushCleanup<PerformSEHFinally>(NormalAndEHCleanup, FinallyFunc, FinallyExecutedFlag);
     return;
   }
 
diff --git a/clang/test/CodeGen/seh-finally-double-execute.c b/clang/test/CodeGen/seh-finally-double-execute.c
new file mode 100644
index 0000000000000..0f2d417e0f4fb
--- /dev/null
+++ b/clang/test/CodeGen/seh-finally-double-execute.c
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -O0 -fms-extensions -fexceptions -fcxx-exceptions -o - %s | FileCheck %s
+
+int freed = 0;
+void myfree(int *p) {
+  ++freed;
+}
+
+// CHECK-LABEL: define dso_local i32 @main(
+int main() {
+  int x = 0;
+  int *p = &x;
+  __try {
+    return 0;
+  } __finally {
+    myfree(p);
+  }
+}
+
+// Check that a guard flag is allocated to prevent double execution
+// CHECK: %finally.executed = alloca i1
+// CHECK: store i1 false, ptr %finally.executed
+
+// Check the main function has guard logic to prevent double execution
+// CHECK: %finally.executed{{.*}} = load i1, ptr %finally.executed
+// CHECK: br i1 %finally.executed{{.*}}, label %finally.skip, label %finally.run
+// CHECK: finally.run:
+// CHECK: store i1 true, ptr %finally.executed
+// CHECK: call void @"?fin$0@0@main@@"
+// CHECK: finally.skip:
+
+// Check the finally helper function is called only once
+// CHECK-LABEL: define internal void @"?fin$0@0@main@@"
+// CHECK: call void @myfree
+// CHECK-NOT: call void @myfree

efriedma-quic · 2025-07-02T22:53:22Z

clang/test/CodeGen/seh-finally-double-execute.c

+  } __finally {
+    myfree(p);
+  }
+}


The current code clang generates looks for this testcase looks fine? In fact, this testcase doesn't have an exception path at all. I'm not sure what you're trying to fix.

(Maybe see also https://reviews.llvm.org/D124642... which is vaguely related.)

Reading the IR without this patch, the RaiseException is called with a call, not an invoke, so any exception should unwind directly to the caller. If we're somehow ending up in the __finally block anyway, that would indicate a problem with the SEH tables generated by the backend. Maybe there's a missing entry in the table?

I don't want to try to work around an backend bug in the clang frontend.

Thanks for the comment! I'll take a look at the tables SEH tables and see if they are being generated correctly

github-actions · 2025-07-02T22:59:19Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions c,cpp -- clang/test/CodeGen/seh-finally-double-execute.c clang/lib/CodeGen/CGException.cpp

View the diff from clang-format here.

diff --git a/clang/lib/CodeGen/CGException.cpp b/clang/lib/CodeGen/CGException.cpp
index f3cbb77d8..bbdad4de4 100644
--- a/clang/lib/CodeGen/CGException.cpp
+++ b/clang/lib/CodeGen/CGException.cpp
@@ -1382,7 +1382,8 @@ namespace {
       // Only execute the finally block if it hasn't already run.
       llvm::BasicBlock *RunFinallyBB = CGF.createBasicBlock("finally.run");
       llvm::BasicBlock *SkipFinallyBB = CGF.createBasicBlock("finally.skip");
-      llvm::Value *AlreadyExecuted = CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
+      llvm::Value *AlreadyExecuted =
+          CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
       CGF.Builder.CreateCondBr(AlreadyExecuted, SkipFinallyBB, RunFinallyBB);
       CGF.EmitBlock(RunFinallyBB);
       CGF.Builder.CreateFlagStore(true, FinallyExecutedFlag);
@@ -1490,11 +1491,12 @@ void CodeGenFunction::FinallyInfo::enter(CodeGenFunction &CGF, const Stmt *body,
   CGF.Builder.CreateFlagStore(false, ForEHVar);
 
   // Allocate a flag to ensure the finally block is only executed once.
-  llvm::Value *FinallyExecutedFlag = CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), "finally.executed");
+  llvm::Value *FinallyExecutedFlag =
+      CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), "finally.executed");
   CGF.Builder.CreateFlagStore(false, FinallyExecutedFlag);
-  CGF.EHStack.pushCleanup<PerformFinally>(NormalCleanup, body,
-                                          ForEHVar, endCatchFn,
-                                          rethrowFn, SavedExnVar, FinallyExecutedFlag);
+  CGF.EHStack.pushCleanup<PerformFinally>(NormalCleanup, body, ForEHVar,
+                                          endCatchFn, rethrowFn, SavedExnVar,
+                                          FinallyExecutedFlag);
 
   // Enter a catch-all scope.
   llvm::BasicBlock *catchBB = CGF.createBasicBlock("finally.catchall");
@@ -1738,14 +1740,17 @@ namespace {
 struct PerformSEHFinally final : EHScopeStack::Cleanup {
   llvm::Function *OutlinedFinally;
   llvm::Value *FinallyExecutedFlag;
-  PerformSEHFinally(llvm::Function *OutlinedFinally, llvm::Value *FinallyExecutedFlag)
-      : OutlinedFinally(OutlinedFinally), FinallyExecutedFlag(FinallyExecutedFlag) {}
+  PerformSEHFinally(llvm::Function *OutlinedFinally,
+                    llvm::Value *FinallyExecutedFlag)
+      : OutlinedFinally(OutlinedFinally),
+        FinallyExecutedFlag(FinallyExecutedFlag) {}
 
   void Emit(CodeGenFunction &CGF, Flags F) override {
     // Only execute the finally block if it hasn't already run.
     llvm::BasicBlock *RunFinallyBB = CGF.createBasicBlock("finally.run");
     llvm::BasicBlock *SkipFinallyBB = CGF.createBasicBlock("finally.skip");
-    llvm::Value *AlreadyExecuted = CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
+    llvm::Value *AlreadyExecuted =
+        CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
     CGF.Builder.CreateCondBr(AlreadyExecuted, SkipFinallyBB, RunFinallyBB);
     CGF.EmitBlock(RunFinallyBB);
     CGF.Builder.CreateFlagStore(true, FinallyExecutedFlag);
@@ -1790,7 +1795,7 @@ struct PerformSEHFinally final : EHScopeStack::Cleanup {
 
     auto Callee = CGCallee::forDirect(OutlinedFinally);
     CGF.EmitCall(FnInfo, Callee, ReturnValueSlot(), Args);
-    
+
     CGF.EmitBlock(SkipFinallyBB);
   }
 };
@@ -2188,9 +2193,11 @@ llvm::Value *CodeGenFunction::EmitSEHAbnormalTermination() {
 void CodeGenFunction::pushSEHCleanup(CleanupKind Kind,
                                      llvm::Function *FinallyFunc) {
   // Allocate a flag to ensure the finally block is only executed once.
-  llvm::Value *FinallyExecutedFlag = CreateTempAlloca(Builder.getInt1Ty(), "finally.executed");
+  llvm::Value *FinallyExecutedFlag =
+      CreateTempAlloca(Builder.getInt1Ty(), "finally.executed");
   Builder.CreateFlagStore(false, FinallyExecutedFlag);
-  EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc, FinallyExecutedFlag);
+  EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc,
+                                         FinallyExecutedFlag);
 }
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {

…com/yasster/llvm-project into user/mmissoum/cg-double-finally-fix

Fixed double finally block execution

2ccf384

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Jul 2, 2025

Merge branch 'main' into user/mmissoum/cg-double-finally-fix

b2da5a7

efriedma-quic reviewed Jul 2, 2025

View reviewed changes

Yassine Missoum and others added 3 commits July 7, 2025 10:58

Updated the test and the code to fix double finally issue

4c99b3f

Merge branch 'user/mmissoum/cg-double-finally-fix' of https://github.…

abf984e

…com/yasster/llvm-project into user/mmissoum/cg-double-finally-fix

Merge branch 'main' into user/mmissoum/cg-double-finally-fix

a55f126

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Fixed double finally block execution #146796

[Clang] Fixed double finally block execution #146796

yasster commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025

Uh oh!

llvmbot commented Jul 2, 2025 •

edited

Loading

Uh oh!

efriedma-quic Jul 2, 2025

Uh oh!

efriedma-quic Jul 8, 2025 •

edited

Loading

Uh oh!

yasster Jul 9, 2025

Uh oh!

github-actions bot commented Jul 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

[Clang] Fixed double finally block execution #146796

Are you sure you want to change the base?

[Clang] Fixed double finally block execution #146796

Conversation

yasster commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025

Uh oh!

llvmbot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasster Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 2, 2025 •

edited

Loading

efriedma-quic Jul 8, 2025 •

edited

Loading

github-actions bot commented Jul 2, 2025 •

edited

Loading