[clang][Interp] Clear metadata when destroying locals

[tbaeder]

Summary

I've had problems with 5b54cf1a2892767fe949826a32d7820732028a38 because we were emitting a Destroy op for a local variable before we were done using that local variable. This moves the local's Block to a DeadBlock but leaves the metadata (a InlineDescriptor in the case of local variables) uninitialized, so we ran into failed builds on the msan builders.

This patch simply zeroes the metadata after calling destroying the local variable. The only difference is that we will now run into an assertion because the Descriptor* is nullptr, etc.

The added code could be in a #ifndef _NDEBUG block, and that would be better for performance of course, but... that wouldn't have helped in the case of the failing msan builders anyway I think.

[aaron.ballman]

clang/lib/AST/Interp/EvalEmitter.cpp

260–261

I'm not certain this is a good idea -- we've just deallocated B and then we're saying "cool, now get me your raw data so I can muck about with it".

The comments in InterpState::deallocate() say // Free storage, if necessary., so this looks a lot like a use-after-free. Am I missing something?

clang/lib/AST/Interp/InterpFrame.cpp

80–81

Same here as above.

[tbaeder]

clang/lib/AST/Interp/EvalEmitter.cpp

260–261

deallocate() doesn't free the Block's memory though, so we can still use it afterwards. That's why I had the problems with https://reviews.llvm.org/rG5b54cf1a2892767fe949826a32d7820732028a38 and neither a
I could also move this code to deallocate directly.

This is just a security measure so we don't end up emitting a load instruction for a variable we've already emitted a destroy instruction for. So just for me, not for users.

[aaron.ballman]

clang/lib/AST/Interp/EvalEmitter.cpp

260–261

Ugh... so we have a function named deallocate that doesn't actually deallocate? Should we rename that?

It's hard to consider it a security measure when it looks so much like a use-after-free. Can we make whatever Desc->DtorFn resolves to do the memset instead (so it's hidden within deallocate() rather than comes after it)?

[tbaeder]

clang/lib/AST/Interp/EvalEmitter.cpp

260–261

Doing it in InterpBlock::invokeDtor() would be a lot cleaner IMO, however, that is only for the data part, not the metadata. That is important for local variables that we reuse in loops; they get their metadata set once and that's it.

I'll just drop this patch since it had limited value anyway.

clang/lib/AST/Interp/EvalEmitter.cpp

 
 bool EvalEmitter::emitDestroy(uint32_t I, const SourceInfo &Info) {
   if (!isActive())
     return true;
 
   for (auto &Local : Descriptors[I]) {
     auto It = Locals.find(Local.Offset);
     assert(It != Locals.end() && "Missing local variable");
-    S.deallocate(reinterpret_cast<Block *>(It->second.get()));
+    Block *B = reinterpret_cast<Block *>(It->second.get());
+    S.deallocate(B);
+    // Just make sure we're runnnig into some kind of error when a
+    // local variable is used after being destroyed.
+    InlineDescriptor &ID = *reinterpret_cast<InlineDescriptor *>(B->rawData());
+    std::memset(&ID, 0, sizeof(InlineDescriptor));
   }
 
   return true;
 }
 
 //===----------------------------------------------------------------------===//
 // Opcode evaluators
 //===----------------------------------------------------------------------===//
 
 #define GET_EVAL_IMPL
 #include "Opcodes.inc"
 #undef GET_EVAL_IMPL

clang/lib/AST/Interp/InterpFrame.cpp

 InterpFrame::~InterpFrame() {
   for (auto &Param : Params)
     S.deallocate(reinterpret_cast<Block *>(Param.second.get()));
 }
 
 void InterpFrame::destroy(unsigned Idx) {
   for (auto &Local : Func->getScope(Idx).locals()) {
     S.deallocate(reinterpret_cast<Block *>(localBlock(Local.Offset)));
+    // Just make sure we're runnnig into some kind of error when a
+    // local variable is used after being destroyed.
+    InlineDescriptor *ID = localInlineDesc(Local.Offset);
+    std::memset(ID, 0, sizeof(InlineDescriptor));
   }
 }
 
 void InterpFrame::popArgs() {
   for (PrimType Ty : Func->args_reverse())
     TYPE_SWITCH(Ty, S.Stk.discard<T>());
 }