Call reset(nullptr) in scoped_ptr's.

base/memory/scoped_ptr.h

Index: base/memory/scoped_ptr.h
diff --git a/base/memory/scoped_ptr.h b/base/memory/scoped_ptr.h
index fb781b0b32ddfd8b47ff3e4a0662ff53b9cd8760..47124d4b6d982a34481916ab5fe1ab4db74d4979 100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -220,6 +220,12 @@ class scoped_ptr_impl {
       // 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.

     }
   }