C/C++ question about thrashing pointers

[IHateMyJob2004]

-------------------- EDIT: The actual code:
Sorry, it is a bit different than in my OP.

Foo::Foo( ... ) {
// These are all class variables.
windowbtn=new MenuButton( ... );
minimizebtn=new Button( ... );
restorebtn=new Button( ... );
maximizebtn=new Button( ... );
}

// Destruct thrashes the pointers
Foo::~Foo(){

windowbtn=(MenuButton*)-1L;
minimizebtn=(Button*)-1L;
restorebtn=(Button*)-1L;
maximizebtn=(Button*)-1L;
}



------------------------ OP:

Well, 3 C/C++ guys coiuldn't figure out what someone did i nthe code we are looking at.

They new a pointer at one point in the code. So, assume a class called Foo. They have:
Foo *foo = new Foo();

Then later on when the want it later, they do not have this:
delete(foo);

Instead, they have:
foo = foo - 1;

The comment that coincides with this is "thrashing pointers".

The Google was powerless in my quest. So I ask you, AT forum member, to tell me what hte heck is going on. The odd thing is that from observing memory usage, I don't think there is a memory leak.

 

[EagleKeeper]

What you are doing is subtracting the object pointer. The compiler handles addition/subtraction of pointers similar to like an array of object items.
subtracting one points to the previous object in the array. If you are at the first array object, then you have removed the object itself from the array, but not out of memory.

For the first foo subtraction, the end result will be the 0 element

If you perform another foo creation before the true deletion, it is possible that the subtraction will result in a pointer to the first foo object dependign on prior memory allocation between the first and second allocations of foo.

 

[IHateMyJob2004]

EDIT: I see you edited .... re-reading your post.

Originally posted by: Common Courtesy
What you are doing is substracting the object pointer.

Yes

For the first foo subtraction, the end result will be 0

If the pointer points to 0x2532, subtracting 1 will jsut give you 0x2531. How do you get zero?

It you perform another foo creation, it is possible that the subtraction will result in a pointer to the first foo object.

The memory is already allocated. If foo1 were never deleted, creating a second pointer (Foo *foo2 = new Foo()) will point to an unallocated section of memory.

 

[IHateMyJob2004]

Originally posted by: Common Courtesy
What you are doing is subtracting the object pointer. The compiler hadles additiona/subtraction of pointers similar to like an array of object items.
subtracting one points to the previous object in the array. If you are at the first array object, then you have removed the object itself from the array, but not out of memory.

For the first foo subtraction, the end result will be 0

If you perform another foo creation before the true deletion, it is possible that the subtraction will result in a pointer to the first foo object dependign on prior memory allocation between the first and second allocations of foo.

If you never call delete on a pointer though, how does the memory get freed?

dereferencing a pointer doesn't clean up anything.

I thin I might have to read up more on what you are talking about though. Any good articles/links?

 

[SunnyD]

Originally posted by: IHateMyJob2004

For the first foo subtraction, the end result will be 0

If the pointer points to 0x2532, subtracting 1 will jsut give you 0x2531. How do you get zero?

No... You get the address of Foo - (1 * sizeof(Foo)). Pointer arithmetic always deals in strides of the size of the type pointed to.

Essentially, you end up with Foo being allocated memory at arbitrary address X. When the code then does Foo = Foo - 1; you end up with Foo pointing to an arbitrary location sizeof(Foo) bytes before Foo. Essentially, you have absolutely no idea what you're pointing at (except in certain specific cases like arrays).

In order to move 1 byte as you are thinking, you would need to cast Foo into a byte pointer, as in:

Foo = (char*)Foo - 1;

 

[tfinch2]

foo = foo - 1 is not minus 1 addressable unit

It's really foo = foo - sizeof(foo)

 

[DaveSimmons]

Using pointer arithmetic is OK if you're traversing a buffer or array and your logic prevents going off either end.

foo = fooarray ; // [0]

foo++ ; // [1]
foo += 10 ; // now [11]
foo = foo - 1 ; // now [10]
foo -= 11 ; // now [ -1 ] -- using may cause page fault or access violation, or may corrupt the heap. Definitely NOT safe.

 

[IHateMyJob2004]

Awesome responses. We just were talking in the lab about what is going on and came up with the same conclusion.

It still leaves dangling pointers though.

I have some code in the original post now. I am even more confused now.

 

[SunnyD]

Originally posted by: IHateMyJob2004
Awesome responses. We just were talking in the lab about what is going on and came up with the same conclusion.

It still leaves dangling pointers though.

I have some code in the original post now. I am even more confused now.
...

Foo::Foo( ... ) {
// These are all class variables.
windowbtn=new MenuButton( ... );
minimizebtn=new Button( ... );
restorebtn=new Button( ... );
maximizebtn=new Button( ... );
}

// Destruct thrashes the pointers
Foo::~Foo(){

windowbtn=(MenuButton*)-1L;
minimizebtn=(Button*)-1L;
restorebtn=(Button*)-1L;
maximizebtn=(Button*)-1L;
}

Uh... you need to be calling delete on windowbtn, minimizebtn, restorebtn and maximizebtn, otherwise you are NOT deallocating the memory for those objects.

Also, if you're explicitly wanted to make a pointer not point to anything valid... assign NULL to it.

The destructor should look like:

Foo::~Foo() {
delete windowbtn;
delete minimizebtn;
delete restorebtn;
delete maximizebtn;

// The following lines are completely unnecessary at this point
// This is the "correct" way of assigning an invalid pointer value
windowbtn = NULL;
minimizebtn = NULL;
restorebtn = NULL;
maximizebtn = NULL;
}

// (Unless you happened to have called delete on them elsewhere in the code
// do NOT delete memory more than once)

There is no point in assigning them to NULL in this case as the object is going out of scope by this time (otherwise it would not be calling the destructor), hence those variables will no longer exist (unless they're static members).

Any time you're using "new", you will need a corresponding "delete" in your code. No exceptions (unless you're using a managed language which handles garbage collection for you).

It sounds to me like you're confused between regular pointers and reference tracking in a managed language - when the reference count in a managed language goes to 0, the memory manager may at that point deallocate the memory for that object as it's no longer in use. C++ is not a managed language, unless you're using managed extensions/smart pointers, which you are not.

 

[tidehigh]

Originally posted by: SunnyD
It sounds to me like you're confused between regular pointers and reference tracking in a managed language

C++ is not a managed language, unless you're using managed extensions/smart pointers, which you are not.

Looks like its a mistake to me, with reasoning quoted.

 

[DaveSimmons]

Right, this looks like either some weird and unreliable way to dereference smart pointers or managed pointers in a .Net., or just an odd way to mark the pointers as no longer valid ("trashing" not "thrashing" though.)

In regular C++ without using a smart pointer scheme you'd want something like:

// Destruct thrashes the pointers -- maybe he meant "trashes" ?
Foo::~Foo(){

delete (windowbtn) ; // free() if malloc not new
windowbtn= NULL ;

}

Setting to NULL is actually a decent idea whenever you delete/free, since then you'll get a more helpful assertion if some code runs after it that uses the freed pointer.

 

[Markbnj]

Well, 3 C/C++ guys coiuldn't figure out what someone did i nthe code we are looking at.

That's some funny code right there. Whoever wrote that should have been fired or sent back to QA.

All the various comments about pointer arithmetic, arrays, etc. are on target. Nobody who doesn't have a pretty thorough understanding of it all should even touch C or C++ code.

But there is no pointer arithmetic going on there, as others have alluded. Somehow, somewhere, the person who wrote that got the idea that the way to free up memory in C++ was to set the pointer to... -1. The compiler was obviously displeased, so they threw in a cast. Problem solved!

First, a pointer is an unsigned scalar type, by definition. You can't have a negative pointer, unless it's a pointer to a region of space under your desk. So what conversions have to take place in that cast for the resulting rvalue to be assigned to, say, windowbtn?

The -1L first has to be converted to an unsigned value. Since it's a dword it will be converted to 0xffffffff, or 4,294,967,295. It then needs to be converted to a pointer. It's pretty obvious that the resulting value points somewhere out past the back case fan. Obviously then, that's a safe filler value for a pointer that you no longer need! Done! Let's get some lunch.

Just kidding. Call delete with those pointers, assign them null, and fire the original author.

 

[IHateMyJob2004]

Sunny,

That's the thing. We are working on someone elses code (and existing code base) and we saw this. We don't understand why, but our memory does not increase as the program runs due to what appears to be an obvious memory leak.

The thing is, none of us have ever seen the code I showed you. And we are kinda staring at walls trying to figure it out (whether or not it is proper).

I guess what I am getting at is if anyone recognizes the technique shown in the example as some sort of rare, uncommon trick in C/C++. All of us i nthe lab think it's busted code.

 

[Crusty]

Originally posted by: IHateMyJob2004
Sunny,

That's the thing. We are working on someone elses code (and existing code base) and we saw this. We don't understand why, but our memory does not increase as the program runs due to what appears to be an obvious memory leak.

The thing is, none of us have ever seen the code I showed you. And we are kinda staring at walls trying to figure it out (whether or not it is proper).

I guess what I am getting at is if anyone recognizes the technique shown in the example as some sort of rare, uncommon trick in C/C++. All of us i nthe lab think it's busted code.

Cause it IS busted code!

 

[SunnyD]

Originally posted by: IHateMyJob2004
Sunny,

That's the thing. We are working on someone elses code (and existing code base) and we saw this. We don't understand why, but our memory does not increase as the program runs due to what appears to be an obvious memory leak.

The thing is, none of us have ever seen the code I showed you. And we are kinda staring at walls trying to figure it out (whether or not it is proper).

I guess what I am getting at is if anyone recognizes the technique shown in the example as some sort of rare, uncommon trick in C/C++. All of us i nthe lab think it's busted code.

Like I said - it looks to me like possibly some sort of assumption that it was using smart pointers. Now again, I am not privy to all of the code, so I can't say either way on that one, but judging by your context, it is NOT using smart pointers. I was going to point something out in my previous post though, but decided not to...

Originally posted by: Markbnj
The -1L first has to be converted to an unsigned value. Since it's a dword it will be converted to 0xffffffff, or 4,294,967,295. It then needs to be converted to a pointer. It's pretty obvious that the resulting value points somewhere out past the back case fan. Obviously then, that's a safe filler value for a pointer that you no longer need! Done! Let's get some lunch.

This may be the reasoning here. -1 = 0xffffffff, which I often times see Visual C++ assigning a new pointer on the stack this value, and in the watch list the value is referenced as "<Bad Ptr>". Keeping in mind it's common to do something like this:

if(!pSomePtr)
pSomePtr = (SomePtrType*)new SomePtrType;

and

if(pSomePtr) {
delete pSomePtr;
pSomePtr = NULL;
}

Those would not be valid if you imply -1 as a boolean value, since it would evaluate to logical true. Neither here nor there, but I wanted to drop that in as VC++ has some of it's own quirks.

Either way, your memory may not "appear" to increase for several reasons - how often are you allocating and deleting Foo? If it's only once, it will only ever allocate the memories allocated in Foo once, but not release it. The key to take away is that while you're not seeing memory leak by repeated allocation, you're also not seeing it be released by proper deallocation - meaning it's a leak. Once Foo goes out of scope, if properly deallocated you should see memory usage go down (again, dependant on several conditions and compiler optimizations).

 

[IHateMyJob2004]

I talked to the current maintainer of the code. He doesn't want to change it because it "works" right now.

It's admitadly a small leak, but when I see things like this I just go nuts when people won't fix it.

 

[Venix]

I can't explain the destructor, but is it possible that MenuButton and Button manage their own memory? For example, creating a MenuButton creates an HWND, and when the HWND is destroyed it deletes the MenuButton.

Or the more likely explanation: It was written by an idiot.

 

[SunnyD]

Originally posted by: Venix
I can't explain the destructor, but is it possible that MenuButton and Button manage their own memory? For example, creating a MenuButton creates an HWND, and when the HWND is destroyed it deletes the MenuButton.

Or the more likely explanation: It was written by an idiot.

I was thinking that too... however I don't even think MFC cleans up explicitly allocated objects like that - I'm not aware of any non-managed windowing API that behaves like that. The only other thing I could think of is if there is some other method that is actually deleting the objects, and the programmer had just assigned the pointers to -1 in the destructor for no apparent reason.

 

[Venix]

MFC doesn't do it automatically, but all you have to do it add "delete this" in PostNcDestroy. In fact, MFC itself does that for some of its own classes (CFrameWnd, I think? I avoid MFC).

More modern frameworks (ex. Qt) do automatically clean up heap objects that are linked to windows or widgets or whatever the hell they're called. It's also a pretty common idiom in Win32; create an HWND, put an object's pointer in a property, then delete it on WM_NCDESTROY.

So I was thinking that maybe the developer wanted that kind of behavior. The MFC built-in class is "CButton", so I am assuming that "Button" is a class from some other framework, or a custom class.

 

[Markbnj]

Yeah, it's a very common solution required by MFC, which is why a search on Google for "*)-1L" returns no documents.

-1 is an invalid value for a pointer. The language provides null for the purpose of setting a pointer to a useless value (and one which is guaranteed to throw an exception if dereferenced). If some mechanism in MFC required or encouraged the programmer to set a pointer to -1 then I've never heard of it.

OP, the guy who says it's working is wrong if it is causing a memory leak. That's not "working", it's slowly failing.

 

[Venix]

Huh? I never defended setting the pointers to -1. I specifically said, "I can't explain the destructor."

I was just suggesting that it might not be leaking memory, since the objects could be freed elsewhere. I then gave some examples of the common idiom of deleting an object when its associated window is destroyed. This code could be doing that, or it could be freeing the objects elsewhere, or it could simply be leaking.

So I agree completely that "ptr = (Ptr*)-1" is nonsense. I would really like to know what the guy was thinking. Or if he was thinking at all.

 

[kylef]

Gotta love programmers who don't understand pointers but try to use them creatively anyway.

 

[Markbnj]

Huh? I never defended setting the pointers to -1. I specifically said, "I can't explain the destructor."

Wasn't directed at you, or anyone in particular. Just the general notion that by some definition that code was proper .

 

[IHateMyJob2004]

I'm not "on the project" where this came up but it is the kind of thing where I'd like to learn the technique or just confirm it is a bug. Bug it is.

Coincidentally I am on another project where the company that wrote some code forgot to delete pointers in destructor. We were adding functionality by mimicking what they did. We got through a couple of units and I realized their error. I tell the software lead about the problem and that we are messing it up to. The response was that "if that's how they do it, that's how we should do it". I was astonished. The person I am dealing with is more Java savvy than C/C++ savvy but I was astonished. I told her that we forgot to clean up the pointers and that will cause a memory leak. "It hasn't caused problems yet so let's just do what they do and leave our code as is". Even more things are bothering me about this person (the fact that I should be promoted and her demoted) based on what I am now realizing.

EDIT: I am mostly pissed because this person has a software lead role. She doesn't know the first thing about designing software (which I am now realizing). She can only mimic code she has seen elsewhere. I am talking about her freaking out over using STL Iterators because the original authors of the code base didn't use iterators. So, my use of Iterators must be wrong (which really pisses me off ... a software lead attacking a co-worker?) She has the gift of gab. Or as it become apparent ... she likes to blather on about nothing. Oh and to top it off, she doesn't understand how to say "I don't know". Only that "you did it wrong". And one swell e-mail about something trivial that I did 100% correctly (object oriented like) but she didn't understand because the Java code she saw did it differently. Due to her own ignorance, she swore at me in the e-mail.

 

[Markbnj]

Get a tool like PC-Lint and integrate it into your testing process, along with something like BoundsChecker. When these complacent types see what these tools produce they might have a come-to-jesus moment.