🔒 Locked

buffer overun with std::map?

Started by danharibo Mar 7, 2009 at 7:04 PM 13 replies 2.6k views

Original Post

Mar 7, 2009 at 7:04 PM

I'm trying to store pointers to member functions in an std::map, but i've hit a bit of a roadblock. I keep getting buffer overuns using this code to copy the functions into the map:


		std::string newName = name;
		std::pair<std::string,InputPointer> inp( newName, point );
		ent->inputs.insert( inp );
		std::cout << "Input " << name << " registered" << std::endl;

the definition of InputPointer is like this:

	typedef void (BaseEntity::*InputPointer)(int);

This is the Macro I used to call the above code:

#define ENT_ADDINPUT( InputName, Pointer )										
	class C##InputName##Foo														
	{																			
	public:																		
		C##InputName##Foo( void )												
		{																		
			std::cout << "address: " << Pointer << " Name: " << #InputName << std::endl;
			std::string nInputName = #InputName;								
			Cell::EntityMap::getEntMap()->addInput(Cell::EntityMap::getEntMap()->getCurrent(), nInputName ,(Cell::InputPointer)Pointer);
		}																		
	};																			
	static C##InputName##Foo mC##InputName##Foo; 

//Here's the Macro Call
	ENT_ADDINPUT( test, &BaseEntity::inputTest );

Anyone know what's going on? Incase you need to know, this is the memory from the std map after I call insert() http://i157.photobucket.com/albums/t62/danharibo/helpimtrapedinapointerfactory.png

alvaro

Mar 7, 2009 at 8:28 PM

Can you try to turn that code into a 10-line example program that we can actually compile and run? Remove everything non-essential to the problem. In the process of reducing the program to the bare minimum that reproduces the problem, you'll often find the problem yourself. If you don't, you can post it here and we can be sure that we are not missing anything important from your code. If the problem is with the compiler, you can submit a very informative bug report.

All advantages. :)

Hodgman

Mar 7, 2009 at 8:47 PM

^^ what he said. We can't really tell what's going on from these snippets.

1) The ENT_ADDINPUT macro generates code that will be run prior to the entry of the main method. Perhaps you have encountered the static initialization order fiasco?
How does the Cell::EntityMap::getEntMap() function work? Does it use the construct on first use idiom, or is the map a static-member/global?

2) The c-style cast of a function pointer (Cell::InputPointer) makes my C++ sense tingle.

danharibo

Mar 8, 2009 at 8:15 AM

Quote:
Original post by Hodgman
^^ what he said. We can't really tell what's going on from these snippets.

1) The ENT_ADDINPUT macro generates code that will be run prior to the entry of the main method. Perhaps you have encountered the static initialization order fiasco?
How does the Cell::EntityMap::getEntMap() function work? Does it use the construct on first use idiom, or is the map a static-member/global?

I don't think so, It's a static function that returns a static class

		static EntityMap* getEntMap() { static EntityMap mMap; return &mMap }

Quote:

2) The c-style cast of a function pointer (Cell::InputPointer) makes my C++ sense tingle.

I removed the cast, (since I'm only using the BaseEntity class right now) and I still get the buffer overun.

The peculiar problem is that the debugger says the data for the std::pair is actually correct, but once it's inserted into the map it disappears.

Kippesoep

Mar 8, 2009 at 8:24 AM

Quote:

		std::string newName = name;		std::pair<std::string,InputPointer> inp( newName, point );		ent->inputs.insert( inp );

This seems overcomplicated. Why not just:

ent->inputs [name] = point;

danharibo

Mar 8, 2009 at 9:15 AM

Quote:
Original post by Kippesoep
Quote:
		std::string newName = name;		std::pair<std::string,InputPointer> inp( newName, point );		ent->inputs.insert( inp );
This seems overcomplicated. Why not just:
ent->inputs [name] = point;

That's what I was doing originally, I still get the buffer overun doing that, but I can see something is wrong. The data inserted into the map using the [] operator becomes corrupted somehow. removing the first 4 characters of the string. Since I used "test" as the input name it was all gone, But when I used "inputtest" the string was cut to "ttest".

rip-off

Mar 8, 2009 at 9:48 AM

Is there any multi-threading going on in your application?

Looking here it appears that you have a pointer to some Win32 heap bookkeeping data.

It is also possible that the map is corrupted before any of this happens. Are you sure the map isn't corrupt?

danharibo

Mar 8, 2009 at 10:15 AM

Quote:
Original post by rip-off
Is there any multi-threading going on in your application?

Looking here it appears that you have a pointer to some Win32 heap bookkeeping data.

It is also possible that the map is corrupted before any of this happens. Are you sure the map isn't corrupt?

I think the actual vector storing the structs with the map in it is getting corrupted:

That's the memory after just 1 entity has been inserted.

rip-off

Mar 8, 2009 at 12:43 PM

What vector? You need to post more code. If you cannot reduce the problem like alvaro suggested, then you are going to find it very difficult to pinpoint the problem, either by yourself or through forums.

Every other piece of code that interacts with this could be causing the problem. If we can't see this code, then we can't diagnose any issues with it.

danharibo

Mar 8, 2009 at 12:57 PM

Quote:
Original post by rip-off
What vector? You need to post more code. If you cannot reduce the problem like alvaro suggested, then you are going to find it very difficult to pinpoint the problem, either by yourself or through forums.

Every other piece of code that interacts with this could be causing the problem. If we can't see this code, then we can't diagnose any issues with it.

It's ok, I found the problem and Fixed it. I'm not quite sure what the problem was though so I'm not sure what It was I did that fixed it

rip-off

Mar 8, 2009 at 1:07 PM

Quote:
Original post by danharibo
It's ok, I found the problem and Fixed it. I'm not quite sure what the problem was though so I'm not sure what It was I did that fixed it

So, you mean you didn't find the problem. [grin]

That is a problem. This kind of thing is a classic case of undefined behaviour - where slight modifications to your program might make it appear to work correctly.

But the bug is still there, latent, and could resurface later on when you make some more changes.

danharibo

Mar 8, 2009 at 1:11 PM

Quote:
Original post by rip-off
Quote:
Original post by danharibo
It's ok, I found the problem and Fixed it. I'm not quite sure what the problem was though so I'm not sure what It was I did that fixed it

So, you mean you didn't find the problem. [grin]

That is a problem. This kind of thing is a classic case of undefined behaviour - where slight modifications to your program might make it appear to work correctly.

But the bug is still there, latent, and could resurface later on when you make some more changes.

I think the Overun was caused by passing the pointer, Once I moved the code that copied it into the macro, the crash went away

Zahlman

Mar 9, 2009 at 11:48 AM

Okay. First, let's expand the macro call (and clean up the formatting - you seem to have ridiculous numbers of tabs at the ends of lines in there):

class CtestFoo {	public:	CtestFoo(void) {		std::cout << "address: " << &BaseEntity::inputTest << " Name: " << "test" << std::endl;		std::string nInputName = "test";		Cell::EntityMap::getEntMap()->addInput(			Cell::EntityMap::getEntMap()->getCurrent(),			nInputName,			(Cell::InputPointer)&BaseEntity::inputTest		);	}};static CtestFoo mCtestFoo;

So what's going on here? We're casting '&BaseEntity::inputTest' to 'Cell::InputPointer', which I assume refers to your previous typedef - so we're casting to 'void (BaseEntity::*)(int)'. It's a BaseEntity member function, so that part is ok.

But. If it takes an int and returns void already, then no cast is necessary. And if it doesn't, I'm pretty sure the cast just plain isn't valid.

danharibo

Mar 9, 2009 at 11:53 AM

Quote:
Original post by Zahlman
Okay. First, let's expand the macro call (and clean up the formatting - you seem to have ridiculous numbers of tabs at the ends of lines in there):

*** Source Snippet Removed ***

So what's going on here? We're casting '&BaseEntity::inputTest' to 'Cell::InputPointer', which I assume refers to your previous typedef - so we're casting to 'void (BaseEntity::*)(int)'. It's a BaseEntity member function, so that part is ok.

But. If it takes an int and returns void already, then no cast is necessary. And if it doesn't, I'm pretty sure the cast just plain isn't valid.

I fixed it already, and the tabs are there because the macro has a "\" at the end of every line, however using tabs and the \ character here breaks the formatting so I removed them

NotAYakk

Mar 9, 2009 at 1:09 PM

Danharibo, you had memory corruption. You moved code that, as far as you know, should have no impact on the memory corruption around. You no longer detected memory corruption.

This means that the memory corruption is now occuring in some spot you cannot detect, or that the undefined behaviour you are triggering is no longer detected by you. It is a huge huge red flag. (or you got lucky)

Unless this is a throw-away project, you shouldn't be happy the problem went away -- you should be scared that you can no longer detect the problem that you still haven't fixed.

In essence, you changed your code so that what was once a reproducible bug, is now an unreproducible bug. This is bad.

Can you get back to the last case where the code generated the memory corruption? Can you engage in systematic reduction of the situation, removing one confounding complication at a time, until either:

A> You find the exact change that you do that makes the memory corruption stop being detected, then prove that the change actually fixes a memory corruption problem, or

B> You reduce the problem to a simple case where you can generate it in a single, small, stand-alone source file. At that point you:

B.1> Understand what is going wrong, and fix it, or

B.2> Show it to someone else, who goes "oh my god, you are doing X, that will screw you".

...

What you have done is detected memory corruption, posted a small part of a complex problem, not found any answers, tweaked something, no longer detected the memory corruption, failed to determine if your tweak actually fixed your bug or just made you no longer able to detect your bug.

That means your code is now in a 'throw it away, it is a half-assed hack' state, not in a 'I am confident this does, and should, work' state. You haven't found the problem. You haven't solved the bug. You have alleviated a possibly easy to diagnose symptom of a bug, from a class of bugs that often generates ridiculously hard to diagnose symptoms, and currently hope that the bug doesn't spawn other random symptoms at some other point where they would screw you completely later on.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.