Skip to content

Conversation

@smanna12
Copy link
Contributor

@smanna12 smanna12 commented Nov 8, 2024

This commit adds an assert statement to the CallBI function to ensure that the interpreter state (S.Current) is correctly reset to the previous frame (FrameBefore) after InterpretBuiltin returns true. This helps catch any potential issues during development and debugging.

@smanna12 smanna12 requested a review from tbaederr November 8, 2024 15:14
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2024

@llvm/pr-subscribers-clang

Author: None (smanna12)

Changes

This commit addresses two static analyzer issues in the CallBI function:

Resource Leak: Ensures that the NewFrame object is properly managed by releasing ownership when InterpretBuiltin returns true, preventing a resource leak.

Use-After-Free: Ensures that S.Current is correctly reset to the previous frame (FrameBefore) after InterpretBuiltin returns true, preventing a use-after-free error.

The changes ensure that the NewFrame object is not prematurely deleted and that the interpreter state is correctly restored in case of failure.


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

1 Files Affected:

  • (modified) clang/lib/AST/ByteCode/Interp.cpp (+7-1)
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index 0e571624ae18d1..dd1236b6d6115d 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -1374,9 +1374,15 @@ bool CallBI(InterpState &S, CodePtr OpPC, const Function *Func,
   S.Current = NewFrame.get();
 
   if (InterpretBuiltin(S, OpPC, Func, CE, BuiltinID)) {
-    NewFrame.release();
+    // Release ownership of NewFrame to prevent it from being deleted.
+    NewFrame.release(); // Frame was deleted already.
+    // Ensure that S.Current is correctly reset to the previous frame.
+    assert(S.Current == FrameBefore);
     return true;
   }
+
+  // Interpreting the function failed somehow. Reset to
+  // previous state.
   S.Current = FrameBefore;
   return false;
 }

@tbaederr
Copy link
Contributor

tbaederr commented Nov 8, 2024

Please reword the title of the PR. It sounds like this commit is actually fixing anything but it just silences the static analyzer.

@smanna12 smanna12 changed the title [clang][bytecode] Fix resource leak and use-after-free issues in CallBI function [clang][bytecode] Improve resource management and state restoration in CallBI function Nov 21, 2024
@smanna12 smanna12 changed the title [clang][bytecode] Improve resource management and state restoration in CallBI function [clang][bytecode] Add assert to ensure correct state restoration in CallBI function Nov 21, 2024
@smanna12
Copy link
Contributor Author

Please reword the title of the PR. It sounds like this commit is actually fixing anything but it just silences the static analyzer.

Thanks @tbaederr for reviews! I have updated title and commit message.

@smanna12
Copy link
Contributor Author

Thank you @tbaederr for reviews!

@smanna12 smanna12 merged commit 95f4aa4 into llvm:main Nov 21, 2024
11 checks passed
@smanna12 smanna12 deleted the my_bug branch November 21, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants