Ogre::SharedPtr

最新推荐文章于 2023-01-05 20:43:19 发布

weixin_34405332

最新推荐文章于 2023-01-05 20:43:19 发布

阅读量99

点赞数

原文链接：https://my.oschina.net/lyr/blog/59222

版权

为什么80%的码农都做不了架构师？>>>

Ogre Forums

Support and community hang-out spot for Ogre3D
http://www.ogre3d.org/forums/

Bug in Ogre::SharedPtr assignment operator

http://www.ogre3d.org/forums/viewtopic.php?f=4&t=33460

Page 1 of 1

Bug in Ogre::SharedPtr assignment operator

Posted: Sat Jun 23, 2007 3:20 pm

by Grahn

The assignment operator in Ogre::SharedPtr calls release(), which indirectly can cause the argument to be destroyed before it is accessed.

How to reproduce:

Code: Select all

struct Foo;
typedef Ogre::SharedPtr<Foo> FooPtr;
struct Foo { FooPtr ptr; };

// In main() or someplace:
    FooPtr foo(new Foo);
    foo->ptr.bind(new Foo);
    foo = foo->ptr;
    // foo now contains a wild pointer!

...or simply examine the source code.

Suggested fix:

Add a swap() function:

Code: Select all

void swap(SharedPtr<T> &other) {
    std::swap(pRep, other.pRep);
    std::swap(pUseCount, other.pUseCount);
}

Do in the following manner in the assignment operator(s):

Code: Select all

SharedPtr& operator=(const SharedPtr& r) {
    SharedPtr<T> tmp(r);
    swap(tmp);
    return *this;
}

I don't have a proper build environment set up at the moment, and I haven't signed any Contributor License Agreement. Otherwise I'd submit a patch myself.

Posted: Sat Jun 23, 2007 3:38 pm

by jacmoe

Just follow the instructions here:
http://www.ogre3d.org/index.php?option=com_content&task=view&id=414&Itemid=142 :wink:

Posted: Sat Jun 23, 2007 4:48 pm

by sinbad

Interesting, we've been using SharedPtr for ages without hitting that particular condition.

Your proposed fix doesn't work, it misses out two things - thread support and incrementing the reference count. After assignment the reference count would actually be unchanged which breaks the SharedPtr. The principle is sound though, I'll put it on the list to fix.

Posted: Sat Jun 23, 2007 5:20 pm

by Grahn

No, I think you are mistaken. Let's take a closer look at what happens in the above example:

Code: Select all

FooPtr foo(new Foo); // Constructor increases count in foo to 1
foo->ptr.bind(new Foo); // bind() increases count in foo->ptr to 1
foo = foo->ptr;
// Enters the operator:
    SharedPtr<T> tmp(r); // Constructor increases count in foo->ptr (alias r) to 2
    swap(tmp); // foo now becomes tmp
    return *this; // Destructor in tmp decreases count in tmp (previously foo) to 0
        // As a result, the data pointed to by tmp is deleted. This includes a SharedPtr (previously foo->ptr).
        // The destructor in that SharedPtr decreases count in foo->ptr (now foo) to 1.

Another way to convince yourself might be to look at where the increments and decrements can happen: The count is increased only in the constructor (well, and in bind(), but that is sort of a construction), and decreased only in the destructor. Thus, the total count is always exactly equal to the number of existing SharedPtr objects. As long as none are leaked, it should be ok.

You could be right about the thread safety, though. I might have misunderstood the intention in the current implementation. My assumption was that any data shared between a set of SharedPtr objects is synchronized, but not the SharedPtr objects themselves. That is, if you do a = b; you must own a and b, but not necessarily c pointing to the same object as a and b. And in that respect my proposed solution should be correct as well, as far as I can see.

Posted: Sat Jun 23, 2007 5:33 pm

by sinbad

Ah yes, I'm wrong - my apologies. I didn't properly interpret the way the temp and swap cooperated, I see why it works now.

That also resolves the thread safety concerns. You are correct that the SharedPtr instance itself should be owned by a single thread and the local copy ensures that the counter is only incremented in a thread-safe manner.

So colour me dumb