Thread: Ban pointers or references on classes?

  1. #16
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by Daved View Post
    >> 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.

  2. #17
    Registered User
    Join Date
    Jan 2005
    Posts
    7,366
    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.

  3. #18
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.

  4. #19
    Registered User
    Join Date
    Apr 2006
    Posts
    2,149
    Quote Originally Posted by Elysia View Post
    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.
    It is too clear and so it is hard to see.
    A dunce once searched for fire with a lighted lantern.
    Had he known what fire was,
    He could have cooked his rice much sooner.

  5. #20
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.

  6. #21
    Registered User
    Join Date
    Jan 2005
    Posts
    7,366
    >> 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.

  7. #22
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.
    Last edited by Elysia; 10-26-2007 at 12:44 PM.

  8. #23

  9. #24
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    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&#37; 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!
    Last edited by iMalc; 10-26-2007 at 02:18 AM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  10. #25
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.
    Last edited by Elysia; 10-26-2007 at 07:47 AM.

  11. #26
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    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.

  12. #27
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.

  13. #28
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    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?

  14. #29
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.

  15. #30
    Registered User
    Join Date
    Jan 2005
    Posts
    7,366
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Pointers v References
    By m37h0d in forum C++ Programming
    Replies: 28
    Last Post: 06-30-2008, 01:29 PM
  2. Replies: 12
    Last Post: 12-31-2007, 06:59 AM
  3. Pointers, Classes, and Errors o my!
    By Scottc1988 in forum C++ Programming
    Replies: 12
    Last Post: 03-13-2003, 10:14 PM
  4. Pointers to derived classes.
    By sean in forum C++ Programming
    Replies: 3
    Last Post: 11-13-2001, 08:19 PM
  5. Pointers to inherited classes
    By sean in forum C++ Programming
    Replies: 1
    Last Post: 11-03-2001, 03:04 PM