deliberately illegal use-after-destroy in llvm/lib/IR/User.cpp

[llvmbot]

Bugzilla Link	24578
Version	unspecified
OS	Linux
Reporter	LLVM Bugzilla Contributor
CC	@eugenis,@morehouse

Extended Description

The test lvm/llvm/test:Transforms/ScalarRepl/sroa-fca.ll.test fails when running it with -fsanitize-memory-use-after-dtor, and environment option MSAN_OPTIONS=poison_in_dtor=1

Invalid access of member HasHungOffUses in definition of operator delete for user. The member is inherited from llvm::Value. During test execution, the destructor of some llvm::Value instance is invoked, and poisons its own memory. The later destruction of the User instance fails when it attempts to access the inherited member.

[eugenis]

To put it simply, operator delete for class User inspects memory of the object after the end of its lifetime. This shows as a use-after-dtor error when running under MemorySanitizer.

There is a similar problem with operator delete for MDNode.

This needs to be fixed or suppressed before we enable use-after-dtor sanitization on the bootstrap bot.

[morehouse]

It looks like a potential solution would be to modify User's overloaded new operators to allocate extra memory to store NumUserOperands, HasHungOffUses, and HasDescriptor. Then the overloaded delete can read that extra memory and delete accordingly without touching the memory poisoned by the destructor.

Of course there are performance implications and likely other difficulties that are outside my current level of understanding.

[llvmbot]

*** Bug llvm/llvm-bugzilla-archive#36022 has been marked as a duplicate of this bug. ***

[llvmbot]

I've been looking at this but I don't have a good solution yet :(

The fact that User objs have stuff -- an array of Uses, or a ptr to a separate Use[] array -- just before it in memory, make it difficult. The trick is knowing where the start of the User's allocation actually is, because operator new allocates extra memory and serves an address after that allocation.

I was hoping the Use[] or Use** could go after the User data but I think that would interfere with subclasses' data.

One thing I did try (which failed) was, like Matt mentioned, stash the needed data in non-poisoned memory. Because it's difficult in User::operator delete to know anything other than where the User* (i.e. the User's this which is being deallocated) address is, simply store the address of the base allocation at this, with something like: new ((void**)this) (void*) (addr), and then read it during operator delete and free it there. No good. Besides adding an additional, (gross) hack, it may introduce another problem, or just exposes some other use-after-free in Value which tries to free its Name.

Some other ideas:

• Let User have a llvm::SmallVector<Use, 2> for operands. It'll have at most 2 operands in-situ, or if more are needed, they'll live externally to that SmallVector. SmallVector has similar outside-the-C++-object-layout shenanigans, with essentially the same goal: reduce allocations if possible. This would make the layout more predictable, easier to understand, less prone to *-SAN aborts.

[llvmbot]

Indeed MDNode has the same pattern and the same issues.

I'll put out another idea then: how about a handful of classes like OwnsFixedStorage<T, N>, OwnsVariableSizedStorage<T>, etc.?

Benefits are:

• encapsulate the ugly details of memory layout to make classes more readable,
• make this easy to integrate in other classes that may want this -- DIExpression has a TODO to this effect
• cut down on redundancy / number of implementations / copypasta
• written with a focus on being MSAN/ASAN/UBSAN/*-SAN clean -- or at least, one place to fix those bugs.

[llvmbot]

There's a class TrailingObjects which is remarkably like this. Except the extra data live after the main object, not before it, in memory.

I'm working on a new class LeadingObjects which will be kind of the counterpart to TrailingObjects but not have all the same kinds of functionality; just parts that I'm factoring out of classes which do this pre-this data thing, like User, MDNode.

[llvmbot]

mentioned in issue llvm/llvm-bugzilla-archive#36022