[PATCH] D37262: The issues with X86 prefixes: step 2

[Andrew V. Tischenko via Phabricator via llvm-commits]

avt77 added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2866
+  if (static_cast<X86Operand &>(*Operands.back()).Kind == X86Operand::Prefix) {
+    X86Operand &Prefix = static_cast<X86Operand &>(*Operands.pop_back_val());
+    Prefixes = Prefix.getPrefix();
----------------
craig.topper wrote:
> avt77 wrote:
> > craig.topper wrote:
> > > I'm not sure this isn't a use after free. pop_back_val is going to return a std::unique_ptr which you dereferenced, but I'm not convinced anything is keeping the std::unique_ptr alive.
> > It seems that everything is OK here because
> > 
> >   C++  Utilities library  Dynamic memory management std::unique_ptr 
> > 
> > typename std::add_lvalue_reference<T>::type operator*() const;  (1)	(since C++11)
> > pointer operator->() const noexcept;                                             (2)	(since C++11)
> > 
> > operator* and operator-> provide access to the object owned by *this.
> > The behavior is undefined if get() == nullptr
> > 
> > Parameters
> > (none)
> > 
> > Return value
> > 1) Returns the object owned by *this, equivalent to *get().
> > 2) Returns a pointer to the object owned by *this, i.e. get().
> > 
> > Am I right?
> The problem isn't with the operator*. It's about the ordering of when the destructor for the std::unqiue_ptr returned by pop_back_val is executed. I believe it will happen as soon as that line ends since it's an unnamed temporary. So the destructor will run and delete the object the unique_ptr is pointing at. But you're still holding a reference to that object. So it's a use after free.
> 
> I think you should assign to Prefixes while the object is still in the vector using .back(), and then once you're done with that just call pop_back() on the vector.
I'm not sure you're right because they say about "end of scope" to invoke destructor and here the "scope" should be "if" statement. But to be absolutely safe I'll do it. Tnx.


https://reviews.llvm.org/D37262