views:

215

answers:

10

Consider this program:

int main()
{
    struct test
    {
        test() { cout << "Hello\n"; }
        ~test() { cout << "Goodbye\n"; }

        void Speak() { cout << "I say!\n"; }
    };

    test* MyTest = new test;
    delete MyTest;

    MyTest->Speak();

    system("pause");
}

I was expecting a crash, but instead this happened:

Hello
Goodbye
I say!

I'm guessing this is because when memory is marked as deallocated it isn't physically wiped, and since the code references it straight away the object is still to be found there, wholly intact. The more allocations made before calling Speak() the more likely a crash.

Whatever the reason, this is a problem for my actual, threaded code. Given the above, how can I reliably tell if another thread has deleted an object that the current one wants to access?

+5  A: 

I was expecting a crash, but instead this happened:

Undefined Behaviour means anything can happen.

Prasoon Saurav
I hate this phrase. While it's technically correct (and in fact a good rule of thumb), I doubt any compiler/architecture combination will make the screen flash with red when certain UB is invoked :)
delnan
@delnan: maybe not red, but I've seen a BSOD from UB :)
JoshD
@JoshD: Hehe... I guess that's why I intuitively didn't write blue :)
delnan
+8  A: 

There is no platform-independent way of detecting this, without having the other thread(s) set the pointer to NULL after they've deleted the object, preferably inside a critical section, or equivalent.

The simple solution is: design your code so that this can't occur. Don't delete objects that might be needed by other threads. Clear up shared resource only once it's safe.

Oli Charlesworth
That's not helpful. It's answering the meta-question.
Paul Nathan
@Paul: It is indeed. But IMHO, this is better advice than simply telling the OP how to fix the immediate symptom. Ending up with code that only conditionally calls functions based on whether they've already been deleted is only going to end up propagating problems.
Oli Charlesworth
And you can use `boost::shared_ptr` to help with this. The manipulation of the reference count is thread-safe.
Steve Jessop
@Paul: I would say, it *is* helpful, because it's answering the meta-question. Since the user doesn't specify an implementation, and there's no portable way to do what the user thinks is necessary, the only way to answer the question without digressing is "You can't". That's not 15 characters so SO won't accept it ;-)
Steve Jessop
I think you guys are missing the point. It sounds like he wants to test if something was deleted elsewhere by causing and catching an exception - also not a great idea but the answer/comments thus far don't address it
Inverse
+2  A: 

Given the above, how can I reliably tell if another thread has deleted an object that the current one wants to access?

How about you set the MyTest pointer to zero (or NULL). That will make it clear to other threads that it's no longer valid. (of course if your other threads have their own pointers pointing to the same memory, well, you've designed things wrong. Don't go deleting memory that other threads may use.)

Also, you absolutely can't count on it working the way it has. That was lucky. Some systems will corrupt memory immediately upon deletion.

JoshD
Doesn't work if the different threads share the object, but don't share the pointer (that is, they each have their own pointer to the same object).
Steve Jessop
@Steve Jessop: Yes. Hence the parenthetical aside addressing that: >(of course if your other threads have their own pointers pointing to the same memory, well, you've designed things wrong.)
JoshD
After deleting but before setting to NULL, thread switch can happen. In that case, bang...
bjskishore123
@bjskishore123: indeed. I see how this could be a catastrophe...
JoshD
@JoshD: how would they share a single pointer? You'd need either a global (not always possible) or a non-shared pointer to that shared pointer.
MSalters
A: 

You could use some static and runtime analyzer like valgrind to help you see these things, but it has more to do with the structure of your code and how you use the language.

David
A: 
// Lock on MyTest Here.
test* tmp = MyTest;
MyTest = NULL;
delete tmp;
// Unlock MyTest Here.

if (MyTest != NULL)
    MyTest->Speak();
Swiss
@Swiss, what's with the `tmp`? Why not `delete MyTest; MyTest = NULL;`?
JoshD
It seems a mistaken assumption. I know some people expect that other threads will see the write to MyTest first, and the effect of the delete later. Hence, if MyTest is _not_ NULL, the object must not have been deleted. That assumes a write and read barrier. The `// Unlock` provides such a write barrier, but there's no read barrier for the `MyTest != NULL` check. Therefore that could operate on stale data in the L1 cache. Of course, once you add proper locking in both threads, the `tmp` becomes redundant.
MSalters
A: 

One solution, not the most elegant...

Place mutexes around your list of objects; when you delete an object, mark it as null. When you use an object, check for null. Since access is serialized, you'll have a consistent operation.

Paul Nathan
+1  A: 

You might use reference counting in this situation. Any code that dereferences the pointer to the allocated object will increment the counter. When it's done, it decrements. At that time, iff the count hits zero, deletion occurs. As long as all users of the object follow the rules, nobody access the deallocated object.

For multithreading purposes I agree with other answer that it's best to follow design principles that don't lead to code 'hoping' for a condition to be true. From your original example, were you going to catch an exception as a way to tell if the object was deallocated? That is kind of relying on a side effect, even if it was a reliable side effect which it's not, which I only like to use as a last resort.

Gabriel
sounds like smart pointers
Ferruccio
yes, boost has them right? I used ref counting in the days of writing COM objects before ATL, so the concept comes to mind first.
Gabriel
The original example is a simple test I wrote to make sure I wasn't going insane, and it doesn't represent my actual program. :-) Your solution sounds good, but what if two threads try to flip the switch to off (since I only need a bool) at the same time? Would there be a danger of the write operations colliding?
Artfunkel
Gabriel
+2  A: 

Despite it's best to improve the design to avoid access to a deleted object, you can add a debug feature to find the location where you access deleted objects.

  • Make all methods and the destructor virtual.
  • Check that your compiler creates an object layout where the pointer to the vtable is in front of the object
  • Make the pointer to the vtable invalid in the destructor

This dirty trick causes that all functions calls reads the address where the pointer points to and cause a NULL pointer exception on most systems. Catch the exception in the debugger.

If you hesitate to make all methods virtual, you can also create an abstract base class and inherit from this class. This allows you to remove the virtual function with little effort. Only the destructor needs to be virtual inside the class.

example

struct Itest
{
    virtual void Speak() = 0;
    virtual void Listen() = 0;
};

struct test : public Itest
{
    test() { cout << "Hello\n"; }
    virtual ~test() { 
        cout << "Goodbye\n";

        // as the last statement!
        *(DWORD*)this = 0;  // invalidate vtbl pointer
    }

    void Speak() { cout << "I say!\n"; }
    void Listen() { cout << "I heard\n"; }
};
harper
+1  A: 

This is not a reliable way to "test" if something has been deleted elsewhere because you are invoking undefined behavior - that is, it may not throw an exception for you to catch.

Instead, use std::shared_ptr or boost::shared_ptr and count references. You can force a shared_ptr to delete it's contents using shared_ptr::reset(). Then you can check if it was deleted later using shared_ptr::use_count() == 0.

Inverse
`shared_ptr` is OK, but reset doesn't force immediate deletion of the resource. It just makes this instance give up the resource. Whether it will be deallocated depends on whether the resource is shared with other `shared_ptr`'s
UncleBens
+3  A: 

I was expecting a crash, but instead this happened:

That is because Speak() is not accessing any members of the class. The compiler does not validate pointers for you, so it calls Speak() like any other function call, passing the (deleted) pointer as the hidden 'this' parameter. Since Speak() does not access that parameter for anything, there is no reason for it to crash.

Remy Lebeau - TeamB
This is interesting as so many other answers suggest that setting your pointer to NULL will help you detect this .. but in this case you don't even need to allocate an object and it still "works" since the pointer is never really used and the compiler just optimises it away. i.e. `main() { test * p = NULL; p->Speak(); }` doesn't crash for me (it _is_ undefined behaviour though)
Michael Anderson
@Michael Anderson: that's rarely useful. For instance, in this case one thread would set _its_ pointer to NULL, but the other thread would happily use its own non-NULL pointer. That's why smart pointer do work: the object destruction is then a consequence of, not a reason for setting a pointer to NULL.
MSalters
@Michael: setting a deleted pointer to NULL only helps detect errors if methods called on the pointer actually access the pointer. That is not the case in this example.
Remy Lebeau - TeamB
Maybe I need to rephrase my response, I'm actually agreeing with Remy, and saying that this particular case is even more pathological, and the suggested answers involving setting p to NULL wont help in anyway for this case.
Michael Anderson