Ban pointers or references on classes?

Originally Posted by Daved

>> Because if somehow the pointer counter reaches 0 while the function is using it, it will use freed memory and crash.
It can't happen. Pretend the function is inline, then move the code from the function into the calling code. You're not calling any functions at that point. If you assume that code without function calls is safe, then code with function calls and pass by reference is safe.

Assuming the constructor(s) increment the count, destructors decrement the count, and copying of the pointer is handled properly, there is no way there can be an issue. If you have a multi-threaded environment, then you need to put some sort of synchronization control around the decrement and delete areas, but that's not a problem for the code that uses the class.

Can't happen you say? I have a situation where it DOES happen. Consider this:

Code:

struct foo_s
{
	ppnew<int> pint;
};

void foo2(pp<int>& pTest)
{
	Sleep(10000);
	int n = *pTest;
	n;
}

void foo3(pp<int> pTest)
{
	Sleep(10000);
	int n = *pTest;
	n;
}

DWORD WINAPI foo(void* pParameter)
{
	foo_s* p = (foo_s*)pParameter;
	foo2(p->pint);
	//foo3(p->pint); // Works if you call this function instead.
	return 0;
}

BOOL CKantanAnimeApp::InitInstance()
{
	foo_s* ps = new foo_s;
	*ps->pint = 100;
	//NewThread(&CKantanAnimeApp::foo, this, ps->pint);
	::CreateThread(NULL, NULL, &foo, ps, NULL, NULL);
	Sleep(1000);
	int n = *ps->pint;
	n;
	delete ps;
	Sleep(100000);
	...
}

When foo2 tries to access the memory, an access violation occours. Uncomment foo3 and comment out foo2 and there will be no access violation. The only difference is that foo2 takes a reference and foo3 takes by value.

At first glance it looks like the problem has nothing to do with passing-by-reference to a function. The same behavior can occur without the function calls:

Code:

struct foo_s
{
	ppnew<int> pint;
};

DWORD WINAPI foo(void* pParameter)
{
	foo_s* p = (foo_s*)pParameter;
	//foo2(p->pint);
	//foo3(p->pint); // Works if you call this function instead.

	{
		Sleep(10000);
		int n = *p->pint; // fails
		n;
	}
	{
		pp<int>& pTest = *p->pint;
		Sleep(10000);
		int n = pTest; // also fails
		n;
	}
	{
		pp<int> pTest = *p->pint;
		Sleep(10000);
		int n = pTest; // succeeds
		n;
	}


	return 0;
}

BOOL CKantanAnimeApp::InitInstance()
{
	foo_s* ps = new foo_s;
	*ps->pint = 100;
	//NewThread(&CKantanAnimeApp::foo, this, ps->pint);
	::CreateThread(NULL, NULL, &foo, ps, NULL, NULL);
	Sleep(1000);
	int n = *ps->pint;
	n;
	delete ps;
	Sleep(100000);
	...
}

The problem is that you are using a pointer in one thread and deleting it in another. That's the fault of passing the pointer to the other thread, which is a separate issue from passing a pointer or reference to another function.

To solve this issue, ps should be passed by value to the thread.

Well, yes, but that's only because I can't pass the pointer by value to the thread function (and start a thread). If I could, I would. That's why I call foo2 or foo3 directly after the thread starts.
You may also notice the commented out NewThread, where I try to pass the memory manager class directly (through reference). Unfortunately, due the inner workings of that code, it doesn't work properly (that's because the code has a local copy of the object passed by value and then passes the object by reference). But that's not really necessary to understand.
This emulates a typical behaviour when several functions are using the same memory (I left out synchronization, though), and when the first function is finished with it, it gets rid of it (or the function exits and calls the destructor).
Don't modify the thead function, modify foo2 or foo3.

Originally Posted by Elysia

It seems there are dents in the armor. pTest2 and pTest3a works.
Here are the current overloaded news:

Code:

	void* operator new(size_t _Count) throw();
	void* operator new(size_t _Count, const std::nothrow_t&) throw();
	void* operator new(std::size_t _Count, void* _Ptr) throw();

A thought: are these "protected" new operator inherited?
ppnew = CMemoryManagerNew: public CMemoryManager

I don't know exactly what all the overloads of new should be. Those three are the basic examples of new that I can think of (nothrow new, array new, and placement new). You can also have several combinations of them.

Operators inheritance works just like method inheritance.

Also necessary to keep in mind that this will effectively prevent your objects from being held in certain implementations of containers. In particular, the STL depends on placement new to copy objects.

I suppose I can do like I've done with CMemoryManagerNew and CMemoryManagerDefault. Create a new class, inherit from CMemoryManager, but override the protected operators with public ones.
Only to be used with things such as STL.

>> This emulates a typical behaviour when several functions are using the same memory
No, it doesn't. It shows how your program can crash when one thread deletes a pointer used by another thread. I still don't see how passing a reference counted smart pointer to a function by reference can ever be dangerous in correct code.

>> Don't modify the thead function, modify foo2 or foo3.
The fact that you call foo2 or foo3 is irrelevant when the code before it is already wrong. The problem is not foo2 or foo3, the problem is that you have two threads accessing the same pointer and one is deleting it. Other than just for fun, you should probably focus on fixing the real source of the problem rather than inventing ways to hide it temporarily.

Alright then, consider this:

Code:

HANDLE hEvent;

void foo2(pp<int>& pTest)
{
	SetEvent(hEvent); // Tell main thread to continue
	Sleep(1000); // Simulate doing some work for 1s
	int n = *pTest;
	n;
}

void foo3(pp<int> pTest)
{
	SetEvent(hEvent); // Tell main thread to continue
	Sleep(1000); // Simulate doing some work for 1s
	int n = *pTest;
	n;
}

DWORD WINAPI foo(void* pParameter) // Wrapper function; takes param and passes it to our processing functions
{
	// Thread entry. Fetch variable we passed along and pass it on to our processing functions.
	ppnew<int>* p = (ppnew<int>*)pParameter;
	foo2(*p);
	//foo3(*p);
	return 0;
}

BOOL CKantanAnimeApp::InitInstance()
{
	//foo_s* ps = new foo_s;
	hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
	ppnew<int> OurMemory; // The memory we're fighting over
	*OurMemory = 100;
	//NewThread(&CKantanAnimeApp::foo, this, ps->pint);
	::CreateThread(NULL, NULL, &foo, &OurMemory, NULL, NULL); // Begin race condition
	WaitForSingleObject(hEvent, INFINITE); // Wait for the new thread to be ready
/*	Sleep(1000); // Simulate some work for 1s*/
	int n = *OurMemory;
	//delete ps;
	OurMemory = ppnew<int>; // We require some new memory now and have no use for the old memory, so allocate some new and discard the old.
	Sleep(100000); // Keep sleeping so the rest of the code won't execute, because it's not part of this example.
	n;
	...
}

Some argument, same code with a somewhat different way of doing it but still the same result. foo2 doesn't work, foo3 does work.

Here are some useful links (they're a bit slow to load):

Chapter 13. Ensure resources are owned by objects. Use explicit RAII and smart pointers

Chapter 25. Take parameters appropriately by value, (smart) pointer, or reference

Chapter 45. Always provide new and delete together

Chapter 46. If you provide any class-specific new, provide all of the standard forms (plain, in-place, and nothrow)

Chapter 93. Avoid using static_cast on pointers

Chapter 95. Don't use C-style casts

I see what you're doing. You're relying on timing to prevent your code from accessing deleted memory.
Due to the Sleep(1000) you can be fairly sure that the copy-constructor implicitly invoked in foo's pass-by-value call to foo3 will have completed before the call to foo3. Since calls to sleep guarantee a minimum sleep time, it will tend to always work. Since foo2 uses a reference, it makes no copy of p, and accesses the deleted memory 9 seconds after it was deleted.

However if you put a Sleep(2000) at the start of the copy-constructor of pp then your call to foo3 will crash as well.
Alternatively, if for some strange reason the CPU was really busy and the code for the copy-constructor was paged out to disk, and another thread or program was hogging the disk IO, then it is concievable that the example as you've posted it will still crash upon calling foo3, because it will have taken too long for the copy-constructor of pp to execute and make that all important copy before deletion.

If the data you're referring to from another thread is going to be deleted and you want to hang onto it, then you obviously have to make a copy before the data is deleted. However you should never be relying on timing to decide when to make that copy.
What you should be doing to prevent such problems is using synchronisation and that means creating and signalling events. Instead of using flakey timing and crosing your fingers, your foo function would make a copy and then signal an event. Meanwhile InitInstance would be waiting for this event to be signalled, and would then be able to 100% safely able to delete the relevent object.
That way you'll also never have to have function like InitInstance that wait plenty of time in case the other thread hasn't made it's copy yet. By signalling and waiting on events your InitInstance function will be able to continue as soon the the other thread is happy, and you will never be left wondering how long of a delay is long enough!

So the problem comes down to your bad threading pratices, NOT anything to do with references, or even C++ for that matter!

No, I used Sleep for simplicity. This is not a real-usage. The Sleep are to indicate (or simulate) that some work is done in those functions, and then they're trying to access the memory.
Remember that OwnMemory stays valid throughout the entire lifetime of the functions, so a reference should not be a bad thing. The only thing that is a bad thing is the reference. By calling by reference, the ref counter won't increase and foo2 will access released memory.
On the other hand, since foo3 passes by value, the ref counter increases and the memory is still valid.

Obviously, this is a race condition, and since I wanted to actually show the problem, I used Sleep to gaurantee that the bad race condition occours. I make sure the memory is deleted before foo2 and foo3 is executed.
I also added some more comments to the sample.

I believe if you switched to using smart pointers (i.e. reference counted), the problem you describe would never happen, because the reference count would be incremented each time the pointer is copied, and it would only be deleted once the reference count reached 0.

That's exactly what it is. But the ref count doesn't increase when you pass it by reference. That's why I want to ban it.

Then don't pass it by reference.

What about manually increasing & decreasing the reference count? Does your smart pointer have some sort of AddRef() & DelRef() methods?

No, it doesn't because that messes everything up and is a prime worry for bugs.
Don't pass it by reference (or by pointer). But is there actually a way to ban it? I'm having trouble with at least one new operator and I can still pass by reference. And as you witness above, it can lead to serious errors.

It still has nothing to do with pass by reference and everything to do with bad coding with threads.

Here is the code again without the functions, exhibiting the same behavior:

Code:

DWORD WINAPI foo(void* pParameter) // Wrapper function; takes param and passes it to our processing functions
{
	// Thread entry. Fetch variable we passed along and pass it on to our processing functions.
	ppnew<int>* p = (ppnew<int>*)pParameter;
	//foo2(*p);
	//foo3(*p);

	{
		Sleep(10000); // Simulate doing some work for 10s
		int n = *p; // FAILS!
		n;
	}
	{
		pp<int>& pTest = *p;
		Sleep(10000); // Simulate doing some work for 10s
		int n = *pTest; // FAILS
		n;
	}
	{
		pp<int> pTest = *p;
		Sleep(10000); // Simulate doing some work for 10s
		int n = *pTest; // SUCCEEDS
		n;
	}

	return 0;
}

BOOL CKantanAnimeApp::InitInstance()
{
	//foo_s* ps = new foo_s;
	ppnew<int> OurMemory; // The memory we're fighting over
	*OurMemory = 100;
	//NewThread(&CKantanAnimeApp::foo, this, ps->pint);
	::CreateThread(NULL, NULL, &foo, &OurMemory, NULL, NULL); // Begin race condition
	Sleep(1000); // Simulate some work for 1s
	int n = *OurMemory;
	//delete ps;
	OurMemory = ppnew<int>; // We require some new memory now
				// and have no use for the old memory,
				// so allocate some new and discard the old.
	Sleep(100000); // Keep sleeping so the rest of the code won't
		       // execute, because it's not part of this example.
	n;
	...
}

Even without the functions, you are sharing memory across threads, and one thread is deleting the memory while the other is using it.

There's no reason (except as a fun exercise) to try to ban use of references to your object.