Double Bug

Zac Howland

I found this in a simulation system last week (simplified version):

// class definition
class Stuff
{
public:
	Stuff() {}
	~Stuff() {}

	void setName(char* name) {_name = name;}
	void setAddress(char* addr) {_address = addr;}
private:
	char* _name;
	char* _address;
};

// usage
char* getName()
{
	char* pRet = new char[20];
	memset(pRet, 0, 20);
	strcpy(pRet, "Charlie");
	return pRet;
}

char* getAddress()
{
	char* pRet = new char[20];
	memset(pRet, 0, 20);
	strcpy(pRet, "United States");
	return pRet;	
}

int main()
{
	Stuff myStuff;
	myStuff.setName(getName());
	myStuff.setAddress(getAddress());

	// do a whole lot of really cool things here
}

HINT: the first bug hides the second one.

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

Rage

Some delete missing somewhere by chance ?

~RaGE();

I think words like 'destiny' are a way of trying to find order where none exists. - Christian Graus

Zac Howland

That is the first bug (memory leak):

int main()
{
	Stuff myStuff;
	char* pTemp = getName();
	myStuff.setName(pTemp);
	delete pTemp;
	pTemp = getAddress();
	myStuff.setAddress(pTemp);
	delete pTemp;

	// do a whole lot of really cool things here
}

Which should reveal the second bug ...

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

ed welch

but why are you are using char arrays instead of the string class?

Zac Howland

This was code given to me to fix. The getXXX() functions are actually 3rd party library calls that cannot be changed. You are very close to the second bug, though.

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

Bugra Barin

If you delete the char arrays right away like that, your Stuff object will have dangling pointers, and you'll have a lot of fun doing those really cool things afterwards :-)

Zac Howland

Exactly! The fix for both bugs looked like this:

// class definition
class Stuff
{
public:
	Stuff() {}
	~Stuff() {}

	void setName(const char* name) {_name = name;}
	void setAddress(const char* addr) {_address = addr;}
private:
	std::string _name;
	std::string _address;
};

// usage
// the following functions were actually part of a 3rd-party library, so they could not be changed
char* getName()
{
	char* pRet = new char[20];
	memset(pRet, 0, 20);
	strcpy(pRet, "Charlie");
	return pRet;
}

char* getAddress()
{
	char* pRet = new char[20];
	memset(pRet, 0, 20);
	strcpy(pRet, "United States");
	return pRet;	
}

int main()
{
	Stuff myStuff;
	char* pTemp = getName();
	myStuff.setName(pTemp);
	delete [] pTemp;
	pTemp = getAddress();
	myStuff.setAddress(pTemp);
	delete [] pTemp;

	// do some really cool stuff here
}

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

Nish Nishant

Zac Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.

Regards, Nish

Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blog

jhwurmbach

Zac Howland wrote:

The fix for both bugs looked like this [...]

Thats not a fix, that is proper programming technique! :-D

"We trained hard, but it seemed that every time we were beginning to form up into teams we would be reorganised. I was to learn later in life that we tend to meet any new situation by reorganising: and a wonderful method it can be for creating the illusion of progress, while producing confusion, inefficiency and demoralisation." -- Caius Petronius, Roman Consul, 66 A.D.

Zac Howland

Nishant Sivakumar wrote:

Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.

The leak was not so subtle. However, the memory violation was more subtle in the original code because the new was called within the 3rd party library (which only the header and a .so was available for). I just simplified the code to make it more obvious (perhaps too obvious).

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

Zac Howland

jhwurmbach wrote:

Thats not a fix, that is proper programming technique!

Agreed; however, isn't that what most bug fixes are? ;P

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

Nish Nishant

Zac Howland wrote:

The leak was not so subtle. However, the memory violation was more subtle in the original code because the new was called within the 3rd party library (which only the header and a .so was available for). I just simplified the code to make it more obvious (perhaps too obvious).

Ah, perhaps it was subtle in the original code. By the way, there's either a leak or a memory violation (since it's the incorrectly placed delete that caused the memory violation). Which of the two was present in your original code?

Regards, Nish

Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blog

Zac Howland

The original code was using Xerces XML parser and calling XMLString::transcode (which you have to read the documentation to realize that it returns a XMLStr* or char* that must be released with XMLString::release). Since the release call was not made, there was a leak (this happened in several places all throughout the code). An object (that resided in another static library) was created and passed the name returned from this transcode function, but the destructor for the object never released any memory (it just held the 2 pointers -- this wasn't initially obvious since the object was in a completely separate library). So when I fixed the memory leak in the library I was working on, we started to see random crashes (access violations) in our simulator. Upon further investigation, the original author of the set of libraries wrote it to store only the pointers, so by fixing the memory leak, I discovered the design flaw. Thus, both were present in the original, but the leak masked the design flaw. Its much easier to see in 30 lines of code or less ... but when you are talking about 5000 lines of code between 3 libraries, its not so obvious.

If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac

Mr Brainley

Nishant Sivakumar wrote:

The leak was glaringly obvious.

Well, all this implicit copying compilers do and stuff like that, i really don't get it, so could you please explain where exactly the leak is ?

Nish Nishant

Mr.Brainley wrote:

Well, all this implicit copying compilers do and stuff like that, i really don't get it, so could you please explain where exactly the leak is ?

A char array is new'd but there's no corresponding delete.

Regards, Nish

Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blog