Skip to content

release/20.x: [IR] Fix assertion error in User new/delete edge case (#129914) #130580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 11, 2025

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Mar 10, 2025

Backport 8d38906

Requested by: @marcauberer

@llvmbot
Copy link
Member Author

llvmbot commented Mar 10, 2025

@llvm/pr-subscribers-llvm-ir

Author: None (llvmbot)

Changes

Backport 8d38906

Requested by: @marcauberer


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

1 Files Affected:

  • (modified) llvm/lib/IR/User.cpp (+6)
diff --git a/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp
index b0aa785deb9af..ab44cb4b8a3f7 100644
--- a/llvm/lib/IR/User.cpp
+++ b/llvm/lib/IR/User.cpp
@@ -146,6 +146,9 @@ void *User::allocateFixedOperandUser(size_t Size, unsigned Us,
   Use *Start = reinterpret_cast<Use *>(Storage + DescBytesToAllocate);
   Use *End = Start + Us;
   User *Obj = reinterpret_cast<User *>(End);
+  Obj->NumUserOperands = Us;
+  Obj->HasHungOffUses = false;
+  Obj->HasDescriptor = DescBytes != 0;
   for (; Start != End; Start++)
     new (Start) Use(Obj);
 
@@ -172,6 +175,9 @@ void *User::operator new(size_t Size, HungOffOperandsAllocMarker) {
   void *Storage = ::operator new(Size + sizeof(Use *));
   Use **HungOffOperandList = static_cast<Use **>(Storage);
   User *Obj = reinterpret_cast<User *>(HungOffOperandList + 1);
+  Obj->NumUserOperands = 0;
+  Obj->HasHungOffUses = true;
+  Obj->HasDescriptor = false;
   *HungOffOperandList = nullptr;
   return Obj;
 }

@tstellar tstellar moved this from Needs Triage to Needs Review in LLVM Release Status Mar 11, 2025
@tstellar tstellar requested a review from nikic March 17, 2025 19:53
@nikic
Copy link
Contributor

nikic commented Mar 17, 2025

I'm not sure this is the right fix. I left a comment on the original PR: #129914 (review)

@tstellar
Copy link
Collaborator

@nikic Should we close this?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay to backport. We should do this properly on main, but this is probably the more conservative fix.

@github-project-automation github-project-automation bot moved this from Needs Review to Needs Merge in LLVM Release Status Apr 11, 2025
Fixes llvm#129900

If `operator delete` was called after an unsuccessful constructor call
after `operator new`, we ran into undefined behaviour.
This was discovered by our malfunction tests while preparing an upgrade
to LLVM 20, that explicitly check for such kind of bugs.

(cherry picked from commit 8d38906)
@tstellar tstellar merged commit e0db588 into llvm:release/20.x Apr 11, 2025
7 of 10 checks passed
@github-project-automation github-project-automation bot moved this from Needs Merge to Done in LLVM Release Status Apr 11, 2025
Copy link

@marcauberer (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@marcauberer
Copy link
Member

@marcauberer (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

This fixes some undefined behaviour when operator delete is called on User after an unsuccessful constructor call
after operator new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants