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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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();
----------------
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.


https://reviews.llvm.org/D37262