Call reset(nullptr) in scoped_ptr's.

base/memory/scoped_ptr.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Scopers help you manage ownership of a pointer, helping you easily manage a
 // pointer within a scope, and automatically destroying the pointer at the end
 // of a scope.  There are two main classes you will use, which correspond to the
 // operators new/delete and new[]/delete[].
 //
 // Example usage (scoped_ptr<T>):
@@ skipping 202 matching lines @@
     // for move-only deleters.
     reset(other->release());
     get_deleter() = other->get_deleter();
   }
 
   ~scoped_ptr_impl() {
     if (data_.ptr != nullptr) {
       // Not using get_deleter() saves one function call in non-optimized
       // builds.
       static_cast<D&>(data_)(data_.ptr);
+      // Even though |this| should no longer be accessed after destruction,
+      // there may be use-after-free bugs. Setting |data_.ptr| to null should
+      // cause many attempts to dereference |this| to segfault closer to the
+      // source of the use-after-free. Of course, this may not catch issues if
+      // the memory is immediately re-allocated and altered.
+      data_.ptr = nullptr;

[danakj, 2015/09/21 21:40:52]

In libc++ they null the member then call the deleter on a temp var:
https://github.com/llvm-mirror/libcxx/blob/master/include/memory

Shall we do the same?

[danakj, 2015/09/21 21:43:08]

I guess that is because they reset() in the destructor instead of using another
destructor. Perhaps we could call reset() in the scoped_ptr destructor and
DCHECK that it's null here instead?

Up to you.

[Anand Mistry (off Chromium), 2015/09/22 01:13:53]

Since we're in the territory of undefined behaviour (read [class.cdtor]), I
think it's better to be explicit and well commented, so I'd like to keep the
code as-is and not call reset(). Even though this is logically equivalent to
calling reset(), I think some of the nuance of making this explicit might be
lost.

[tapted, 2015/09/22 02:53:09]

(drive-by): the null-before-delete strategy is to handle cycles - the call to
delete above could conceivably destroy |this|, making the `data_.ptr = nullptr`
step a UAF.

However, the way to break a cycle is with reset() and reset() already has a
null-before-delete. I couldn't contrive an example where a loop is created AND
that loop can be broken via the destructor rather than with reset()  (I think
the only way to do this would be to semantically have a double-free, and trying
to cater for it might turn a semantic double-free into a "safe-but-not-really"
`delete nullptr`).

So I don't *think* null-before-delete is needed here, and one could probably
argue that for performance reasons it should be skipped. But there might be
something I've missed.

[Anand Mistry (off Chromium), 2015/09/22 03:14:15]

This is the defined behaviour for unique_ptr::reset for exactly the reason you
state.
But we're in the destructor here. Deleting |this| in the destructor is even more
in the realm of undefined behaviour and itself a significant bug. If you run
into a situation where the object's deleter deletes |this|, you also potentially
have a double-free of |this|, aside from the potential use-after-free that is
being introduced.

     }
   }
 
   void reset(T* p) {
     // This is a self-reset, which is no longer allowed for default deleters:
     // https://crbug.com/162971
     assert(!ShouldAbortOnSelfReset<D>::value || p == nullptr || p != data_.ptr);
 
     // Note that running data_.ptr = p can lead to undefined behavior if
     // get_deleter()(get()) deletes this. In order to prevent this, reset()
@@ skipping 350 matching lines @@
 scoped_ptr<T> make_scoped_ptr(T* ptr) {
   return scoped_ptr<T>(ptr);
 }
 
 template <typename T>
 std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
   return out << p.get();
 }
 
 #endif  // BASE_MEMORY_SCOPED_PTR_H_