0

What should happen when the following program is executed?

#include <iostream>
#include <memory>

class test;
std::shared_ptr<test> a_test_object;
struct test
{
    ~test()
    {
        std::cout << "destroy test" << std::endl;
        auto ptr = a_test_object;
    }
};

int main()
{
    a_test_object = std::make_shared<test>();
    //a_test_object.reset();  // Uncomment this and it works fine.
}

I tested this on both GCC and Visual Studio 2015 and in both cases the program crashes. What happens is the shared pointer decrements the count in its destructor, then ~test() is executed, which copies the shared pointer incrementing and then decrementing the count, triggering an infinite recursion of calls to ~test(). Oddly enough calling reset() doesn't trigger the problem.

I ran into this today because some old code that used a pre-C++11 version of shared_ptr, which doesn't have this double deletion bug, was updated to use std::shared_ptr. To my surprise, std::shared_ptr made the program crash. Is this really the intended behavior of std::shared_ptr?

curiousguy
  • 8,038
  • 2
  • 40
  • 58
Davis King
  • 4,731
  • 1
  • 25
  • 26
  • 11
    The only way you can enter the destructor `~test` is if the lifetime of `a_test_object` has ended, and so accessing that object after its lifetime has ended is UB. – Kerrek SB Oct 28 '17 at 15:05
  • Yeah, I guess. This seems like a belligerent adherence to the standard in this case. – Davis King Oct 28 '17 at 15:10
  • 2
    I'm not seeing the belligerence angle here. What would you expect shared_ptr to do in this case? – Peter Ruderman Oct 28 '17 at 15:18
  • I expect it to free the test object once. – Davis King Oct 28 '17 at 15:19
  • 6
    I don't see how that could work. Once you're in your destructor, the ref count on the shared_ptr's control block is 0. Then you assign it to the local ptr variable which ups the ref count to 1 again. When it goes out of scope, the count drops to 0 again and you get your second free. There's no way for shared_ptr to know you've been up to shenanigans. – Peter Ruderman Oct 28 '17 at 15:21
  • It's easy. The shared_ptr fees the object *before* it decrements the count. The old non-standard shared_ptr I was using prior to this did that and it works properly. – Davis King Oct 28 '17 at 15:30
  • 2
    Once it has been decided that an object is to be destroyed, it is not expected that your code somehow access the `shared_ptr` so nobody should care if the count is decremented before or after... If it makes your code fails, **then your code is wrong** and should be rewritten. The above code makes no sense at all and code review in your team should discover such bugs... – Phil1970 Oct 28 '17 at 15:39
  • This is an example that illustrates the issue. The real code where I encountered this issue was in ancient Win32 event callback logic with execution paths in and out of windows event routines. It was much larger than this toy example and involved a much more complex sequence of steps where this wasn't at all apparent. I mean, we tell people std::shared_ptr can deal with complicated patterns of things pointing to other things. It's irritating that things that could be made to work don't for pedantic reasons. – Davis King Oct 28 '17 at 15:45
  • 3
    @DavisKing "std::shared_ptr can deal with complicated patterns of things pointing to other things" - of course it can't, not in that generality. It can provide shared ownership, that's all. – lisyarus Oct 28 '17 at 15:46
  • 4
    @DavisKing: "*I mean, we tell people std::shared_ptr can deal with complicated patterns of things pointing to other things.*" No, we don't. Complicated patterns of pointing often lead to *circular references*, which `shared_ptr` absolutely cannot handle. – Nicol Bolas Oct 28 '17 at 15:53
  • @Phil1970 "_so nobody should care if the count is decremented before or after_" This is not correct. The destruction can only occur when the last owner drop its ownership which means the count MUST be zero. – curiousguy Oct 30 '17 at 14:25
  • 1
    @NicolBolas There is nothing special about "circular references". shared_ptr handles all cases correctly, as specified: the managed object destruction begins when the last owner drops its ownership (when it is destroyed). That this may never happen is not a problem with shared_ptr, but with the design of the program. – curiousguy Oct 30 '17 at 14:28
  • @curiousguy: You're just being pedantic, ignoring the distinction between "what the type does" and "what the user wanted/expected the type to do." `shared_ptr` is for cleaning up objects that are owned by multiple owners. Therefore, cases where they fail to do so are not being handled "correctly" by any useful definition of that term. – Nicol Bolas Oct 30 '17 at 15:04
  • @NicolBolas There is no failure. What you consider "not correctly" (never destroying anything in a dep. cycle) is *the strict respect of the shared_ptr guarantee*. Circular dependency is always a design issue. It will create all kinds of other practical issue if people try to recurse on the data structure not knowing there is a cycle! – curiousguy Oct 31 '17 at 04:41
  • The issue *might be* that some people have been told that destructors, smart ptr, ref counting... was the moral equivalent and the replacement of the GC in Java or SML. It isn't, and the GC doesn't provide the replacement for dtors either. Stroustrup explained it well (somewhere, don't remember where). – curiousguy Oct 31 '17 at 04:45
  • 1
    @curiousguy: "*What you consider "not correctly" (never destroying anything in a dep. cycle) is the strict respect of the shared_ptr guarantee.*" Only because a smart pointer that can handle cycles would be [prohibitively expensive](https://stackoverflow.com/a/35029742/734069). If they could give `shared_ptr` the ability to handle cycles without incurring massive overhead and/or other usability problems, they would have. – Nicol Bolas Oct 31 '17 at 04:46

1 Answers1

8

You have violated a basic rule of C++ object lifetime: you cannot access an object after its lifetime has ended. The deletion of test can only be achieved if all shared_ptr instances have ended their lifetimes. Therefore, its destructor, and all code it calls, cannot access any shared_ptr instances that refers to itself.

To do otherwise invokes undefined behavior. This is not a bug in shared_ptr; it's a bug in your code. It's just as much UB if test were an object in a vector<test> and tried to access that vector<test> instance in your destructor.

It's irritating that things that could be made to work don't for pedantic reasons.

This is not a "pedantic reason". It is impossible. a_test_object doesn't exist anymore; it's not a valid object.

It doesn't work for the same reason that returning a reference to a stack variable doesn't work. The object is gone at this point; you can't access it anymore.

So you'll need to restructure your code to deal with that fact.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • "_you cannot access an object after its lifetime has ended._" Is that a library rule or a core langage rule? – curiousguy Oct 30 '17 at 14:34
  • 3
    @curiousguy: Since the destructor of `a_test_object` is called by the destructor of `shared_ptr`, further use of that `shared_ptr` instance falls under [class.cdtor]. The language rule ultimately boils down to "it depends on the object." And since the standard does not define the state of its library objects currently under destruction, their state is by definition undefined. And therefore, accessing them results in undefined behavior. – Nicol Bolas Oct 30 '17 at 15:03
  • Your comment is more accurate than your answer. The shared_ptr's destructor is under execution. Its members are assuredly intact. Its storage is assuredly intact. To claim that "it doesn't exist anymore" and "it's not a valid object" doesn't seem quite right. Observations about the limited things you can do _during_ dtor execution (as you allude to in your comment) are far more compelling. Consider a rewrite? :) – Lightness Races in Orbit Mar 26 '19 at 16:37