[PATCH] D142277: [clang][Interp] Clear metadata when destroying locals

[Timm Bäder via Phabricator via cfe-commits]

tbaeder abandoned this revision.
tbaeder marked an inline comment as done.
tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/EvalEmitter.cpp:260-261
+    // local variable is used after being destroyed.
+    InlineDescriptor &ID = *reinterpret_cast<InlineDescriptor *>(B->rawData());
+    std::memset(&ID, 0, sizeof(InlineDescriptor));
   }
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > 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?
> > `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.
> 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)?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142277/new/

https://reviews.llvm.org/D142277