Can the assignment of a shared_ptr trash the `this` pointer

Question

Let's take the following example of a data structure (Node) that represents a tree of child nodes. The set of child nodes for each object is stored in a map>

class Node;
typedef std::shared_ptr<Node> NodePtr;

class Node
{
    std::map<const std::string, NodePtr> _childNodes;
    void SomeOtherMethod();

public:
    bool GetChildByKeyName(/*In*/ const std::string& key, /*Out*/ NodePtr& spChild)
    {
        bool result = false;
        auto itor = _childNodes.find(key);
        if (itor != _childNodes.end())
        {
            spChild = itor->second;
            result = true;
            SomeOtherMethod();
        }
        return result;
    }
};

And the following code sample as users would typically call it.

NodePtr spNode, spChildNode;
bool result;
...
result = spNode->GetChildByKeyName(strChildKeyName, spChildNode);

So far so good.

It occurred to me that callers might way to traverse the tree without having to deal with extra variables for each depth into the tree

NodePtr spNode;
bool result;

result = spNode->GetChildItem(strChildKeyName, spNode);
if (result)
   spNode->GetChildItem(strSubKeyName, spNode);

In the above case, if spNode is the final remaining reference to the object, then I'm worried about this block of code in the GetChildItem method:

            spChild = itor->second;
            result = true;
            SomeOtherMethod();

Does the assignment of spChild (which is really the caller's spNode instance) inadvertently destruct "this" Node since the last reference just went away? (And hence calling other methods after the spChild assignment is dangerous). Do I have a potential bug here?

I think the workaround is to simply add this line at the top of the method call:

NodePtr spChildRef = spChild; // maintain a reference to the caller's original node during the scope of the method

Thoughts?

If it does trash, then it would be the weirdest, perhaps hardest to detect `delete this;` I'll ever encounter. — Mark Garcia, Mar 05 '14 at 06:58
@Mat - spNode would be the root node in this particular case with a refcounf of 1. — selbie, Mar 05 '14 at 07:04

score 6 · Accepted Answer · answered Mar 05 '14 at 07:20

You are correct that if the outermost spNode pointer in your second example is the only reference to the root item, GetChildByKeyName will replace that reference causing the object to destruct (essentially "delete this").

I realize this might not be the full code and there may be reasons for why you have designed it like this, but I would personally suggest to change the interface to return the found child instead of using an out parameter. (You can still distinguish between success & failure in finding the child by testing for null.)

Not only does the actual find code become simpler:

NodePtr GetChildByKeyName(/*In*/ const std::string& key)
{
    auto itor = _childNodes.find(key);
    if (itor != _childNodes.end())
    {
        SomeOtherMethod();
        return itor->second;
    }
    return nullptr;
}

You can then also reuse the pointer to your heart's content:

NodePtr spNode; 
....

spNode = spNode->GetChildItem(strChildKeyName);
if (spNode)
    spNode = spNode->GetChildItem(strSubKeyName);

Thanks! For whatever reason this simple idea never occurred to me. I have a long history with C++, but at the start of my career, the old-school C++ gurus taught me to never return objects via the stack so as to avoid copy-constructor overhead, stick with pointers as out params, etc... My current product group is a bit more progressive. So this idea works even better. — selbie, Mar 05 '14 at 07:56

score 3 · Answer 2 · edited May 23 '17 at 12:09

+1 to @Dentoid's answer. I'm not going to duplicate his answer here. I'll only show if your existing code has a problem.

Can the assigment of a shared_ptr trash the this pointer?

Yes, it does.

I have made a test to determine it: http://coliru.stacked-crooked.com/a/ef0d4f92902b4dee

Its output (formatted for clarity):

spNode before: 0x15a1028
   this: 0x15a1028 <---------------------------------------------------------|
   spChild.get() == this                                                     |
                                                                             |
   spChild before: 0x15a1028 <-------- Notice: They're the same -------------|
      Node 0x15a1028 destroyed.                                              |
             ^---------------------------------------------------------------|
   spChild after: 0x15a1078                                                  |
                                                                             |
   SomeOtherMethod() @0x15a1028; size: 1 |                                   |
spNode after: 0x15a1078     ^------------------------------------------------|

Node 0x15a1078 destroyed.

So SomeOtherMethod() is called on an already destroyed object, though the runtime hasn't been able to detect this.

Applying your work-around, http://coliru.stacked-crooked.com/a/f0042d4b46fed340

spNode before: 0x19a7028
   this: 0x19a7028
   spChild.get() == this

   spChild before: 0x19a7028
   spChild after: 0x19a7078

   SomeOtherMethod() @0x19a7028; size: 1

   Node 0x19a7028 destroyed.
spNode after: 0x19a7078

Node 0x19a7078 destroyed.

It doesn't have the problem anymore.

Thanks Mark. I gave you an upvote for validating this. You confirmed what I had suspected. — selbie, Mar 05 '14 at 07:57
Just wanted to note that "though the runtime hasn't been able to detect this" is a bit misleading. It's normally not really about any runtime detecting anything, but about the memory pointed to by "this" getting released to be reused by other things. It may work (since it's likely not overwritten yet in this case), it may not, but using the memory is at that point undefined and needs to be avoided. — Dentoid, Mar 05 '14 at 08:17

Can the assignment of a shared_ptr trash the `this` pointer

2 Answers2